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

feat: introduce a sensor abstraction #474

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

ROMemories
Copy link
Collaborator

Description

This is the first step in providing a sensor abstraction/API whose goal is to provide consistent access to sensor drivers and their readings, abstracting over implementation details of specific sensor devices. It is a (partial) implementation of RFC #201 (handling of thresholds and sensor interrupts will come later and is expected to be relatively orthogonal).

The main change is the introduction of a riot-rs-sensors crate, which is re-exported in riot-rs as the sensors module, which is feature-gated behind the sensors Cargo feature. Until this API and the first sensor drivers are landed, this feature is considered experimental/unstable and breaking changes will therefore not be reflected on the version number of the riot-rs crate.

Issues/PRs references

Partial implementation of RFC #201.

Open Questions

  • Should we derive serde::Serialize on the relevant items?
  • Are floats completely out of the question, even as helpers? Integers should stay at the core of the API, but maybe we could have additional (feature-gated?) helpers on Value+ReadingAxis+Accuracy which would return floats taking into account everything required to make sense of the reading (in particular the scaling value)?
  • The Reading trait was initially introduced when it was thought it would be possible to return different concrete types from Sensor::wait_for_reading() (it now always return a Values); should the trait be removed and its methods implemented on Values itself, slightly simplifying the API (it is not impossible that Rust would allow returning different concrete types from Sensor::wait_for_reading() in the far future)?
  • Open for bikeshedding:
    • We should agree on names for what is currently called:
      • Sensor category: could be renamed to sensor class to match SAUL naming, but "class" is a reserved keyword in many languages.
      • Scaling (value): could be "scaling", "prescaler", something else?
    • Some of the documentation is split between the top module, the Value and the Accuracy types, in an attempt to keep it close to its associated implementation. Should we instead group (some of) it in the top-level module?
    • The set of items we re-export at root level could use some refinement (or err on the side of caution and not re-export much for now).

Future work

The following steps would be:

  • Adding support for external interrupts triggered by sensor devices: physical event interrupts and data available interrupts are to be treated differently even though they use the same pins.
  • Adding the first sensor drivers provided by RIOT-rs (the current API does also allow to provide and use third-party sensor drivers developed for this sensor API): drivers for push buttons, some temperature sensors and 3-axis accelerometers exist in a preliminary state but are not part of this PR.
  • Introducing codegen for automating the instantiating and configuration of sensor drivers based on static, structured configuration files.

How to review this PR

I think this PR is better reviewed by generating the rustdoc docs locally and starting from there, before glancing at the implementation.

TODO

  • The Category, Label, and MeasurementUnit enums should be expanded with more variants, even though these can (and will) be expanded later.

Change checklist

  • I have cleaned up my commit history and squashed fixup commits.
  • I have followed the Coding Conventions.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

@ROMemories ROMemories marked this pull request as ready for review October 18, 2024 13:36
#[derive(Debug, Copy, Clone, PartialEq)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub struct Value {
value: i32,
Copy link
Collaborator Author

@ROMemories ROMemories Oct 22, 2024

Choose a reason for hiding this comment

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

Should we downsize this to an i24 (?) so we can losslessly convert it to an f32?

Comment on lines +14 to +15
/// [Acceleration *g*](https://en.wikipedia.org/wiki/G-force#Unit_and_measurement).
AccelG,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given this isn't a SenML unit, should we remove it and ask sensor drivers to convert it to m/s² instead?

@ROMemories ROMemories added the sensor api The sensor API label Oct 25, 2024
@bergzand
Copy link
Collaborator

Some initial comments based on the docs

Bikeshedding:

  • I would prefer Sample instead of Value.
  • To prevent confusion I would call the different quantities returned by a sensor different channels and not axes, so Sensor::reading_channels(). Mostly to remove ambiguity when dealing with accelero + gyro sensors.
  • I think category is good enough, but I agree that there might be a better name.

Does it make sense to include the sensor range in a same way the accuracy is provided? I think this can be valuable with gas sensors and accelerometers, to know when the measurement is out of (or close to) the max range.

Are floats completely out of the question, even as helpers? Integers should stay at the core of the API, but maybe we could have additional (feature-gated?) helpers on Value+ReadingAxis+Accuracy which would return floats taking into account everything required to make sense of the reading (in particular the scaling value)?

I think a set of (gated) helpers to convert to floats is useful, even if it just to make printing measurements easier.

On the API side, the only question I have so far is whether it is easy enough to zip a set of Values and their ReadingAxis metadata. But there's probably some Rust thingy for that (zip probably :)

@ROMemories
Copy link
Collaborator Author

Bikeshedding:

  • I would prefer Sample instead of Value.

👍 To be clear, that would mean in the case of 3-axis accelerometers we'd have three samples, part of one reading.

  • To prevent confusion I would call the different quantities returned by a sensor different channels and not axes, so Sensor::reading_channels(). Mostly to remove ambiguity when dealing with accelero + gyro sensors.

Currently a 3-axis accelerometer + magnetometer (like the LSM303AGR on the micro:bit V2) would return six Values/Samples and have six axes (X, Y, Z of the accelerometer, and X, Y, Z of the magnetometer). The physical quantity these correspond to is given by the associated ReadingAxis::label(), which returns a Label.

The plan was to expand that Label enum to disambiguate between the values/samples of the acceleremoter/magnetomer (e.g., by adding AccelX, AccelY, AccelZ and MagX, MagY, MagZ, or some other names (possibly not abbreviated) we can bikeshed later). Your remark made me reconsider whether we'd actually be able to group these, without introducing any memory overhead; I need to give it some thought, thanks!

Does it make sense to include the sensor range in a same way the accuracy is provided? I think this can be valuable with gas sensors and accelerometers, to know when the measurement is out of (or close to) the max range.

That's indeed something I haven't considered yet; I'll look into how useful this could be and whether we can do that while keeping memory usage under control, thanks for the suggestion!

Are floats completely out of the question, even as helpers? Integers should stay at the core of the API, but maybe we could have additional (feature-gated?) helpers on Value+ReadingAxis+Accuracy which would return floats taking into account everything required to make sense of the reading (in particular the scaling value)?

I think a set of (gated) helpers to convert to floats is useful, even if it just to make printing measurements easier.

👍 I don't this needs to be part of this first PR, as I expect the helpers to be purely additive.

On the API side, the only question I have so far is whether it is easy enough to zip a set of Values and their ReadingAxis metadata. But there's probably some Rust thingy for that (zip probably :)

It sure is! There's even a pointer to that already in the docs of ReadingAxes::iter() (and yes, Iterator::zip() is the right one).

@bergzand
Copy link
Collaborator

👍 To be clear, that would mean in the case of 3-axis accelerometers we'd have three samples, part of one reading.

Exactly!

Currently a 3-axis accelerometer + magnetometer (like the LSM303AGR on the micro:bit V2) would return six Values/Samples and have six axes (X, Y, Z of the accelerometer, and X, Y, Z of the magnetometer). The physical quantity these correspond to is given by the associated ReadingAxis::label(), which returns a Label.

The plan was to expand that Label enum to disambiguate between the values/samples of the acceleremoter/magnetomer (e.g., by adding AccelX, AccelY, AccelZ and MagX, MagY, MagZ, or some other names (possibly not abbreviated) we can bikeshed later). Your remark made me reconsider whether we'd actually be able to group these, without introducing any memory overhead; I need to give it some thought, thanks!

(Don't forget the 9 axis IMU's, those include Rot{X,Y,Z}.)

The way I interpreted the current ReadingAxis as a way to distinguish multiple Samples from a single sensor device. So the bme680 has three axes/channels, each requiring some label.

Does it make sense to include the sensor range in a same way the accuracy is provided? I think this can be valuable with gas sensors and accelerometers, to know when the measurement is out of (or close to) the max range.

That's indeed something I haven't considered yet; I'll look into how useful this could be and whether we can do that while keeping memory usage under control, thanks for the suggestion!

Are floats completely out of the question, even as helpers? Integers should stay at the core of the API, but maybe we could have additional (feature-gated?) helpers on Value+ReadingAxis+Accuracy which would return floats taking into account everything required to make sense of the reading (in particular the scaling value)?

I think a set of (gated) helpers to convert to floats is useful, even if it just to make printing measurements easier.

👍 I don't this needs to be part of this first PR, as I expect the helpers to be purely additive.

Agreed, and this can all be added later, don't want to introduce too much feature creep here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sensor api The sensor API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants