Skip to content
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

Merged
merged 7 commits into from
Mar 21, 2024

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Jan 23, 2022

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

  • Build on any supported board, use /.well-known/core for discovery and fetch any sensor value from a resource indicated there, eg. /saul/0:
$ aiocoap-client 'coap://[2a02:b18:c13b:8014:fcb1:c71b:a2bb:bd4]/.well-known/core'
application/link-format content was re-formatted
<>; ct="0"; title="Landing page",
</time>; ct="0"; title="Clock",
</poem>; sz="1338",
</ps/>; title="Process list",
</vfs/>; ct="0",
</saul/>; title="SAUL index",
</saul/0>; rt="tag:chrysn@fsfe.org,2020-10-02:saul",
</saul/1>; rt="tag:chrysn@fsfe.org,2020-10-02:saul",
</saul/2>; rt="tag:chrysn@fsfe.org,2020-10-02:saul",
</saul/3>; rt="tag:chrysn@fsfe.org,2020-10-02:saul",
</saul/4>; rt="tag:chrysn@fsfe.org,2020-10-02:saul",
</saul/5>; rt="tag:chrysn@fsfe.org,2020-10-02:saul",
</saul/6>; rt="tag:chrysn@fsfe.org,2020-10-02:saul"
$ aiocoap-client 'coap://[2a02:b18:c13b:8014:fcb1:c71b:a2bb:bd4]/saul/6'
2425×10^-2 °C

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]

@github-actions github-actions bot added the Area: examples Area: Example Applications label Jan 23, 2022
@chrysn chrysn added Area: CoAP Area: Constrained Application Protocol implementations Area: Rust Area: Rust wrapper CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jan 23, 2022
@github-actions github-actions bot removed the Area: CoAP Area: Constrained Application Protocol implementations label Jan 23, 2022
@chrysn chrysn added the Area: CoAP Area: Constrained Application Protocol implementations label Jan 23, 2022
@github-actions github-actions bot removed the Area: CoAP Area: Constrained Application Protocol implementations label Jan 24, 2022
@chrysn
Copy link
Member Author

chrysn commented Jan 24, 2022

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).

@chrysn chrysn added the Area: CoAP Area: Constrained Application Protocol implementations label Jan 24, 2022
@github-actions github-actions bot removed the Area: CoAP Area: Constrained Application Protocol implementations label Jan 28, 2022
@chrysn
Copy link
Member Author

chrysn commented Jan 28, 2022

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).

@chrysn chrysn marked this pull request as ready for review January 28, 2022 15:24
@chrysn
Copy link
Member Author

chrysn commented Jan 30, 2022

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.

@chrysn
Copy link
Member Author

chrysn commented Feb 25, 2022

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.
@chrysn chrysn removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 25, 2022
@chrysn
Copy link
Member Author

chrysn commented Feb 25, 2022

Rebased; disabled CI readiness until the original questions around this are answered.

@stale
Copy link

stale bot commented Nov 2, 2022

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.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 2, 2022
@chrysn
Copy link
Member Author

chrysn commented Nov 2, 2022

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.]

Copy link
Contributor

@Teufelchen1 Teufelchen1 left a 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.

examples/rust-gcoap/Makefile Outdated Show resolved Hide resolved
@Teufelchen1
Copy link
Contributor

I had to run rustup target add thumbv7em-none-eabihf first before I could attempt to compile it. The issue with that is, that the helpful message telling me to do so was hidden behind a wall of compilation errors. If you happen to have an idea on how to improve that situation...

I still can not compile this tho:

error[E0554]: `#![feature]` may not be used on the stable release channel
 --> /home/teufelchen/.cargo/registry/src/index.crates.io-6f17d22bba15001f/coap-message-0.2.2/src/lib.rs:2:1
  |
2 | #![feature(generic_associated_types)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the attribute
  |
  = help: the feature `generic_associated_types` has been stable since `1.65.0` and no longer requires an attribute to enable

Probably a version mismatch? Can you check up on it?

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Mar 19, 2024
@chrysn
Copy link
Member Author

chrysn commented Mar 19, 2024

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…

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 19, 2024
Copy link
Contributor

@Teufelchen1 Teufelchen1 left a 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?

@riot-ci
Copy link

riot-ci commented Mar 19, 2024

Murdock results

✔️ PASSED

48d4e8a Merge branch 'master' into rust-gcoap-add-saul

Success Failures Total Runtime
13 0 14 01m:01s

Artifacts

@Teufelchen1 Teufelchen1 added this pull request to the merge queue Mar 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2024
@Teufelchen1
Copy link
Contributor

I think you need to blacklist some boards... :D

@chrysn
Copy link
Member Author

chrysn commented Mar 21, 2024

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.

@chrysn chrysn added this pull request to the merge queue Mar 21, 2024
Merged via the queue into RIOT-OS:master with commit d10fa1b Mar 21, 2024
25 checks passed
@chrysn chrysn deleted the rust-gcoap-add-saul branch March 21, 2024 01:07
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: examples Area: Example Applications Area: Rust Area: Rust wrapper CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants