Skip to content

Fix availability and naming of properties and state structs for devices#276

Merged
heliosfa merged 8 commits intodevelopmentfrom
BUGFIX-0231-typenames
Dec 16, 2021
Merged

Fix availability and naming of properties and state structs for devices#276
heliosfa merged 8 commits intodevelopmentfrom
BUGFIX-0231-typenames

Conversation

@heliosfa
Copy link
Copy Markdown
Contributor

@heliosfa heliosfa commented Jul 8, 2021

This PR:

The plate heat example has been updated, and a very simple compilation test added, in POETSII/Orchestrator_examples#14.

Vol III B has had some minor updates: docx|pdf

APSP tweaks:

To get the APSP example to compile, I had to add #include <climits> to the shared code.

I also had to add a const unsigned cast to the second parameter in the calls to std::min to handle:

error: no matching function for call to 'min(const unsigned int&, long unsigned int)'
unsigned curr_k = std::min(K, graphProperties->total_vertices - deviceState->round_base);

There is still a warning that I am ignoring for now:

warning: comparison of integer expressions of different signedness: 'int32_t' {aka 'long int'} and 'unsigned int' [-Wsign-compare]
      if(0 <= id_delta && id_delta < K){

@heliosfa heliosfa added bug Report of a bug feature New feature refactor Refactor source to make future work easier/possible composer Issues related to the Composer labels Jul 8, 2021
@heliosfa heliosfa self-assigned this Jul 8, 2021
@heliosfa heliosfa requested review from m8pple and mvousden July 10, 2021 11:47
Copy link
Copy Markdown
Contributor

@mvousden mvousden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks largely sane to me. More thoughts:

  • The fix for #232 is good - splitting those structure declarations off into their own header file will make debugging a fair amount easier.

  • Given that application writers are permitted to access these types (as this is a user-facing feature), I think they should be introduced in the application documentation, perhaps with a corresponding example handler 🍠.

  • I think the application writer should be told that their message payloads are packed tightly - another documentation update perhaps?

  • Your APSP changes also seem sane, though I would suggest addressing the signedness warning (just use a cast in the condition?).

  • Since you're fixing #232, can we un-xfail the APSP test? These are denoted in the Bash scripts in Tests/ReferenceXML - they call TODO functions instead of success or failure functions. Do let me know if you need guidance with this. 🍠

  • Where I put the 👍, I wondered if requestIdle had been implemented. If so, please "activate" the corresponding tests in the same way as the above. 🍠

@heliosfa
Copy link
Copy Markdown
Contributor Author

heliosfa commented Aug 4, 2021

  • Given that application writers are permitted to access these types (as this is a user-facing feature), I think they should be introduced in the application documentation, perhaps with a corresponding example handler 🍠.
  • I think the application writer should be told that their message payloads are packed tightly - another documentation update perhaps?

I'll do a docs tweak and PR.

  • Your APSP changes also seem sane, though I would suggest addressing the signedness warning (just use a cast in the condition?).

Interestingly, I now cannot reproduce this warning.

  • Since you're fixing Types for device properties and state only available within that devices handlers #232, can we un-xfail the APSP test? These are denoted in the Bash scripts in Tests/ReferenceXML - they call TODO functions instead of success or failure functions. Do let me know if you need guidance with this. 🍠
  • Where I put the 👍, I wondered if requestIdle had been implemented. If so, please "activate" the corresponding tests in the same way as the above. 🍠

I have enabled both of these tests.

@heliosfa
Copy link
Copy Markdown
Contributor Author

heliosfa commented Dec 7, 2021

Turns out that I had done the docs changes ages ago but forgot to PR them...

@heliosfa heliosfa requested a review from mvousden December 7, 2021 20:40
Copy link
Copy Markdown
Contributor

@mvousden mvousden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing further to add from prior review, looks good to me.

@heliosfa heliosfa merged commit e3a7600 into development Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Report of a bug composer Issues related to the Composer feature New feature refactor Refactor source to make future work easier/possible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Types for device properties and state only available within that devices handlers Standard type-name aliases are not emitted

2 participants