Skip to content

Conversation

2bndy5
Copy link
Member

@2bndy5 2bndy5 commented Apr 24, 2025

Concrete Error type

Changes the Esb* traits to require a new trait RadioErrorType.

pub trait EsbRadio: RadioErrorType {

In the RadioErrorType trait, there is an associated Error type.

/// A trait to alias hardware related errors about the radio.
pub trait RadioErrorType {
type Error;
}

Then, the RadioErrorType trait is implemented for the RF24 struct.
impl<SPI, DO, DELAY> RadioErrorType for RF24<SPI, DO, DELAY>
where
SPI: SpiDevice,
DO: OutputPin,
DELAY: DelayNs,
{
type Error = Nrf24Error<SpiError, OutputPinError>;
}

Subsequent required implementations

The ? operator used to propagate Result::Err needed an explicit forwarding mechanism for hardware-related errors, namely embedded_hal::digital::ErrorKind as OutputPinError and embedded_hal::spi::ErrorKind as SpiError.

impl From<SpiError> for Nrf24Error<SpiError, OutputPinError> {
fn from(value: SpiError) -> Self {
Nrf24Error::Spi(value)
}
}
impl From<OutputPinError> for Nrf24Error<SpiError, OutputPinError> {
fn from(value: OutputPinError) -> Self {
Nrf24Error::Gpo(value)
}
}

This allows automatic categorization of the error encountered using our Nrf24Error enum.

Advantages

This convoluted approach

  • allows the radio implementation to choose their own error type (useful if/when I get around to implementing ESB API on nRF5x chips).
  • better prepares for implementing the network layers since only the radio type needs to be known at compile time.
  • provides better error messages when debugging panics
  • unifies the error type returned by various Esb* traits' fallible functions. Previously, each trait had it's own associated error type which didn't bode well for implementing different radio hardware.

Cached TX address

For the TX address bug, see nRF24/RF24#1028 (and corresponding solution in nRF24/RF24#1029).

Renamed private fields

Field and variable names prefixed with _ tells the rust linter ("clippy" tool) that unused dead code shall be explicitly ignored. This does not bode well for static analysis on production code.

Coming from Arduino, this isn't the case. Arduino lib convention suggests to prefix private members with a _ for readability. Many C++ linters don't treat the _ prefix of member/variable/parameter names in any special way (excluding #include guards).

2bndy5 added 3 commits April 24, 2025 12:27
For TX address bug, see nRF24/RF24#1028.

Field and variable names prefixed with `_` tells the rust linter ("clippy" tool) that unused dead code shall be explicitly ignored.

Coming from Arduino, this isn't the case. Arduino lib convention suggests to prefix private members with a `_` for readability.
Copy link

codecov bot commented Apr 24, 2025

Codecov Report

❌ Patch coverage is 97.52066% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.81%. Comparing base (33fe013) to head (cb72ce1).
⚠️ Report is 34 commits behind head on main.

Files with missing lines Patch % Lines
crates/rf24-rs/src/radio/rf24/mod.rs 90.47% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main      #38      +/-   ##
===========================================
- Coverage   100.00%   99.81%   -0.19%     
===========================================
  Files           22       22              
  Lines         3154     3194      +40     
===========================================
+ Hits          3154     3188      +34     
- Misses           0        6       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +47 to +57
impl From<SpiError> for Nrf24Error<SpiError, OutputPinError> {
fn from(value: SpiError) -> Self {
Nrf24Error::Spi(value)
}
}

impl From<OutputPinError> for Nrf24Error<SpiError, OutputPinError> {
fn from(value: OutputPinError) -> Self {
Nrf24Error::Gpo(value)
}
}
Copy link
Member Author

@2bndy5 2bndy5 Apr 25, 2025

Choose a reason for hiding this comment

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

These implementations cannot be covered with unit tests. Doing so would require changing the test harness to employ qemu, but that seems like overkill since

  1. the emulated hardware would be infallible under a proper hardware setup
  2. the qemu instance would need to interact with another qemu instance to emulate wireless transactions. I'm guessing this approach is not a thing we can do currently.

EDIT: This could be resolved upstream in the embedded-hal-mock library.

Copy link
Member Author

@2bndy5 2bndy5 Apr 25, 2025

Choose a reason for hiding this comment

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

I opened a couple issues to hopefully resolve this upstream in the mock library we're using for unit tests:

Alternatively, I can fork embedded-hal-mock and tell cargo to pull in my patched fork from git source instead of the original from crates.io. Although, this would incur a tech debt just to satisfy 100% line coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a locally stashed patch that will use my fork's patches (in my fork's dev branch). I'm waiting for feedback upstream before committing the patch. Local tests show these lines will be covered with these upstream patches

@2bndy5
Copy link
Member Author

2bndy5 commented May 2, 2025

Too many unrelated changes in this PR. I've broken it up into 3 separate contributions.

@2bndy5 2bndy5 closed this May 2, 2025
@2bndy5 2bndy5 deleted the error-n-cache-fixes branch May 4, 2025 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant