-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
examples/rust-gcoap: Add SAUL #17554
examples/rust-gcoap: Add SAUL #17554
Conversation
7b3871e
to
c088079
Compare
c088079
to
ecd1f3a
Compare
Keeping this as a draft for some time -- the SAUL server has learned GET and PUT with CBOR objects equivalent to those of phydat's JSON, but for them to be easily available a few maintenance releases all around need to happen (e.g riot-wrappers wrapping phydat_unit_to_str_verbose, and coap-handler-implementations providing the glue code for the minicbor crate that replaces the deprecated serde_cbor). |
6f3be16
to
36274b3
Compare
36274b3
to
765b8bd
Compare
I'd say this is ready for review now. I'll still hope to advertise the resources better (with details on the kind of SAUL device), but to get that I'll need to make a bit of progress on the CoRAL side of things to make sure we don't run into difficulties when migrating away from link-format. (And I'll yet need to extend the board exclusion list, for it's running into RAM limits -- and/or make progress where it comes to finding which components blow up in resource use). |
765b8bd
to
b6e953e
Compare
Rebased onto master as #17591 (fixed in master now) impeded diagnostics run on this that'd bring more light to exactly why the stack requirements are as they are. |
This will need to be rebased after #17704 (swallowing two commits). Possibly it should also consider the new SenML additions that have been made. |
Since 9503809, a relatively recent version of riot-wrappers is required.
On microbit-v2, getting .well-known/core would otherwise result in a stack overflow. Consequently, some boards were removed from the list of supported boards as the currently required RAM exceeds their capacity.
b6e953e
to
40efa06
Compare
Rebased; disabled CI readiness until the original questions around this are answered. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
This just needs the patches refreshed and decisions on the two open questions; pulling in reviwers for those before doing the updates. [edit: The updating is a bit stalled on this branch having sen me on a yak shaving tour through static stack size assessment, which when turned on on LLVM starts producing countless linker errors, possibly because some code in RIOT pulls in code from headers of undeclared module dependencies and then relies on the linker GC to not scream earlier.] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to test it & form an opinion on the open questions.
I had to run I still can not compile this tho:
Probably a version mismatch? Can you check up on it? |
The version mismatch comes from that when this was written, we still had nightly as a default and in our CI, and now don't have it any more. Merging in master will solve that, WIP… |
Makefile.ci was completely removed, pending regeneration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Regarding your open questions: I don't think you'll get an answer in this PR. Maybe move it to an issue or feature them in a weekly meeting?
My opinion on question 1.): I think this is fine. If we are scared to provide this minimal usability because sEcUrItY what does that say about our confidence in our testing, reviewing and security policies? 🤡
2.) is better answered on a summit/vma?
I think you need to blacklist some boards... :D |
93df18d
to
668823b
Compare
668823b
to
6acbebb
Compare
d57844c
to
48d4e8a
Compare
The previous Makefile.ci was regenerated while I was doing some last fix. The latest version has a regenerated Makefile.ci, and merges in master once more resolving Cargo.lock conflicts. |
Contribution description
The Rust Gcoap example already uses two modules from the riot-coap-handler-demos crate from the riot-module-examples:
/ps/
and/vfs/
.This adds a third,
/saul/
, which gives some rudimentary access to SAUL devices over the network. Write support is currently limited, but present eg. for RGB LEDs.Some other maintenance like updating the versions, increasing the gcoap stack's RAM as it was already too little and fixing deprecations happens in separate commits.
Testing procedure
/.well-known/core
for discovery and fetch any sensor value from a resource indicated there, eg./saul/0
:Open questions
Are we OK with actions being taken over the network in a more-than-read-only fashion? (Or: How much do we trust that SAUL devices writable in our boards actually do no harm?)
For CoAP discovery it's good to have resource types; the only ones we can use without IANA action are full URIs, and the tag URIs above are those easiest to mint for me.
As the community controlling riot.org we can use that domain for creating both tag and other URIs (with the former ideal for more ephemeral uses like when we don't really know where we're going yet); do we have any prior expertise in stable-identifier management here?
Related issues
This removes 9 boards from the list supported on this example, as ROM size is exceeded.
Whereas for ROM overflows #17515 already has approaches (which would bring the calliope board, which also exceeds ROM size, back under the threshold, but doesn't change anything for RAM), the topic of the CoAP wrappers' stack usage need more eyes. Given this was already broken before at least for some boards / requests, that should probably be a different issue.
[edit: Added note on removed boards]