Improvements for the CXD5602PWBIMU and IMU add-on board

[btpsj]

Recent work landed (#14) providing support for the CXD5602PWBIMU and corresponding IMU add-on board. The example is functional but can be improved with various aspects, so I'm documenting some of potential changes here. I don't have a board so I'll defer changes to later, since some aspects my have larger API changes that need validation on the hardware itself.

• refactor(imu): renaming structs to have capital I
  Some structs have a capital I like PwbImuParts but others have it lowercase Pwbimu. This should be changed to be consistent and just use capital I across all
• refactor(imu): separate sensor from board
  The pwbimu.rs module is big, and holds two distinct components. One is for the sensor and the other is for the Spresense-specific implementation. To make it easier to manage, splitting these into two separate modules would be ideal. A large refactoring could also split the sensor code out to be its own crate. This would make it easier to reuse, testing, compile-times without necessarily being coupled to depending on the spresense-bsp.
• feat(imu): improve ergonomics around generic type parameters
  Many of the structs and functions have several generic type parameters, and some functions require skipping a clippy lint to allow for complicated type signatures. This, I expect, can be difficult to work with by hand, and also cause really large type errors if things are wrong. Having to skip the clippy lint as well points to needing to shift to some other design. Some different strategies to mitigate this are: grouping the GPIO pins under a trait and Pins struct, making the state transitions be from state-specific structs instead of encoding in a type parameter.
• feat(imu): builder for enabling sensor
  The current api has power_on, enable, and configure methods. Instead of needing to carry out this sequence, and forgetting some part, it would be easier and less error-prone to combine things together, and apply the configuration using a builder struct.
• fix(imu): switch imu example to uart_alt
  The *_alt.rs modules are intended to replace the the current set of peripherals, which don't have the new API around clock configuration.
• refactor(imu): switch example to let-else
  There are few cases in the example for the error paths where it seems like it would be more ergonomic to use a let-else block, especially since fail returns !, and cleanup some verbose match ... { Ok(s) => s, ... blocks.
• feat(imu): flatten error types
  Similar to the type complexity mentioned above, but not necessarily an improvement. There are two ways to handle errors, which is adding a generic type parameter, which makes passing the full error information downstream easier, but the types get complicated, especially if you have an error type that has multiple errors like Error which handles errors across I2C, SPI, and GPIO. Another strategy is using a thiserror and #[from] to flatten the Error. There is some potential information loss, but worth looking into and making the error type a nicer to work with. There are also some functions like power_off that ignore the GPIO warnings using let _ = pin.set_high(); and it would be easier to support failing when the Error types don't clog up the type signature and need more clippy lint skips
• refactor: make imu state tokens structs
  Not necessarily needed change, but generally, for typestate markers, struct Token is used, and not enum Token {}. While there are some small differences, going with the convention would be the ideal, or at the very least document why there is this difference.
• refactor: i2c address as enum
  The I2C address for the IMU sensor is passed as just u8. The general convention leans more towards enums with options for switching.

[nakajimayoshi]

Hey Brandon,

Thanks for the feedback. I still have the add-on board with me so I'll get on these changes today.

Some comments regarding the proposed changes:

refactor(imu): separate sensor from board
The pwbimu.rs module is big, and holds two distinct components. One is for the sensor and the other is for the Spresense-specific implementation. To make it easier to manage, splitting these into two separate modules would be ideal. A large refactoring could also split the sensor code out to be its own crate. This would make it easier to reuse, testing, compile-times without necessarily being coupled to depending on the spresense-bsp.

I agree there is a boundary between the host-agnostic CXD5602PWBIMU driver and the Spresense-specific wiring constructor, and extracting the latter into an internal module could make that clearer. I don’t think “sensor
versus board” is quite accurate though, since the generic portion drives the complete PSoC-fronted add-on board rather than the underlying sensor. A module split would be organizational only; dependency and compile-time isolation would require a separate crate. I’d prefer to address that larger extraction separately when there is a concrete non-Spresense consumer, but given the name of the project I would expect most implementations to have first class support for spresense boards over other consumers. A completely abstract "Imu" trait would be better suited in another repository.

feat(imu): improve ergonomics around generic type parameters
Many of the structs and functions have several generic type parameters, and some functions require skipping a clippy lint to allow for complicated type signatures. This, I expect, can be difficult to work with by hand, and also cause really large type errors if things are wrong. Having to skip the clippy lint as well points to needing to shift to some other design. Some different strategies to mitigate this are: grouping the GPIO pins under a trait and Pins struct, making the state transitions be from state-specific structs instead of encoding in a type parameter.

The main purpose of these type parameters to leverage Rust's type system to make compile-time guarantees that the consumers are using the appropriate wiring for the PSoC IMU board. For the most common use-case (a cxd5602 chip) a spresense-specific driver function is provided which performs the type inference automatically. Other chip consumers would only need to define the IO generics once in their driver structure, and then use the existing spresense driver convenience constructor as a reference to build an ergonomic way of consuming the pins and & buses for their downstream consumers. The complexity of the type parameters has more to do with the complexity of the IMU add on-board itself; it's very advanced compared a typical consumer-grade standalone imu sensor. You can only move the complexity in this case to another part of the code, but it's not possible to remove it all together.

feat(imu): builder for enabling sensor
The current api has power_on, enable, and configure methods. Instead of needing to carry out this sequence, and forgetting some part, it would be easier and less error-prone to combine things together, and apply the configuration using a builder struct.

The type-state pattern guarantees that consumers won't have to worry about finding these errors after flashing the board, they'll be caught by the compiler. But otherwise I agree, this setup can be condensed into a builder. It would then be up to the consumers to perform pattern matching on the elided errors during setup if they miss a critical build step.

refactor: make imu state tokens structs
Not necessarily needed change, but generally, for typestate markers, struct Token is used, and not enum Token {}. While there are some small differences, going with the convention would be the ideal, or at the very least document why there is this difference.

Empty enums prevent marker values from being constructed and emphasize that the states only exist at the type level.

  pub enum Off {}

means “Off exists only as a type; no Off value can exist.” That matches the implementation because the state appears exclusively as PhantomData.

  pub struct Off;

means “Off is a constructible zero-sized token.” The standard may be to use structs here but there hasn't yet been a concrete reason for why these tokens should be constructed by consumers. Making them structs may only add to the confusion of how the driver API is consumed, so I wanted it to be clear that these are Type-level markers only. I'll add this explanation to the documentation.

[nakajimayoshi]

@btpsj Adding onto this another place for improvement for the IMU module is adding a method that gives the caller full control over the IMU data allocation, rather than returning an owned type for every call. Also makes it consistent with the GNSS HAL.

[btpsj]

I agree there is a boundary between the host-agnostic CXD5602PWBIMU driver and the Spresense-specific wiring constructor, and extracting the latter into an internal module could make that clearer. I don’t think “sensor versus board” is quite accurate though, since the generic portion drives the complete PSoC-fronted add-on board rather than the underlying sensor. A module split would be organizational only; dependency and compile-time isolation would require a separate crate. I’d prefer to address that larger extraction separately when there is a concrete non-Spresense consumer, but given the name of the project I would expect most implementations to have first class support for spresense boards over other consumers. A completely abstract "Imu" trait would be better suited in another repository.

On the other hand, if there isn't a good driver for a non-Spresense consumer to use then nobody will try to use it. Once someone gets to the point of willing to use the IMU addon board without the Spresense, they'd probably just write it themselves anyways. One of my goals isn't only adding support for a single Spresense-specific Rust HAL but make the hardware and all add-ons compatible with the broader Rust embedded-hal community and also look towards non-Spresense+NuttX C applications via C FFI. Its a bit more ambitious and agree its building something without consumers, so I'll pick up any of this extra effort (I also find this kind of fun too anways), but also since this is all bare-metal and not tied to NuttX there is a good opportunity to make things more portable. I have some incoming commits that do a similar thing with the camera board, and planning to make it similarly overly portable once I get some of the necessary equipment in.

The main purpose of these type parameters to leverage Rust's type system to make compile-time guarantees that the consumers are using the appropriate wiring for the PSoC IMU board. For the most common use-case (a cxd5602 chip) a spresense-specific driver function is provided which performs the type inference automatically. Other chip consumers would only need to define the IO generics once in their driver structure, and then use the existing spresense driver convenience constructor as a reference to build an ergonomic way of consuming the pins and & buses for their downstream consumers. The complexity of the type parameters has more to do with the complexity of the IMU add on-board itself; it's very advanced compared a typical consumer-grade standalone imu sensor. You can only move the complexity in this case to another part of the code, but it's not possible to remove it all together.

I understand the goal is to add compile-time guarantees, but the its still worth exploring improving ergonomics, like when someone uses the wrong types, how easy is it to figure out what you did wrong, without sacrificing the compile-time guarantees. Having different structs versus adding a generic type parameter can have the same compile-time guarantees, but change the kinds of type errors so I plan to explore how those changes help with that in the future. I haven't tried it with the imu driver so I need to validate first, but in my experience if you pass it the wrong type with structs that have all of these type parameters, its can be hard to figure out what the problem was, but maybe that has improved so it doesn't need to be fixed.