Skip to content

Conversation

2bndy5
Copy link
Member

@2bndy5 2bndy5 commented May 2, 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.

Using dev branch of embedded-hal-mock fork

There were a few lines about forward error types into our own Nrf24Error enum (impl From<T> -- see above).
I opened a couple issues to hopefully resolve this upstream in the mock library we're using for unit tests:

For now, I'll be using my fork's dev branch until these patches are accepted and deployed upstream.

Copy link

codecov bot commented May 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (39d8287) to head (da90c7a).
⚠️ Report is 35 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #42   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         3154      3175   +21     
=========================================
+ Hits          3154      3175   +21     

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

@2bndy5 2bndy5 marked this pull request as ready for review May 2, 2025 09:06
@2bndy5 2bndy5 merged commit 72fe862 into main May 2, 2025
43 checks passed
@2bndy5 2bndy5 deleted the concrete-error branch May 2, 2025 18:13
@2bndy5 2bndy5 added the enhancement New feature or request label Aug 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant