Inner error handling for different error types

[jhodapp]

The purpose of this thread is to discuss and decide on a preferred approach to error handling from deep within the crate that allows for different classes of errors to be passed and mapped upward to the highest level API (i.e. the Wifi struct and the highest level error type, Error).

Right now we've had a single kind of error called ProtocolError that's used in spi.rs.

In adding a new error to signify that doing a DNS query has failed, we realized that it may be time to allow for inner errors to be different types instead of mixing seemingly unrelated ones in the same enum.

Please share any and all constructive ideas.

[calebbourg]

One way that we could think of this is a distinction between what users of the crate can control and what they can't. Or, in other words, given a certain error situation. Is there something users can change to troubleshoot the error? Discrepancies in firmware versions on the WIFI module can be corrected. Whether or not the correct NINA firmware command is invoked is entirely up to the authors of this crate to correct. We don't expose access to that control. So maybe we can reconsider the concept of protocol level errors as they are not actionable by users.

[jhodapp]

Is there something users can change to troubleshoot the error?

We don't expose access to that control. So maybe we can reconsider the concept of protocol level errors as they are not actionable by users

These are very insightful points Caleb, thank you for them. I agree with your premise. Not every action needs to be actionable (sometimes a user still might want to know something if it caused malfunctioning internal state, even if to file a bug report, but obviously we don't try and set out to intentionally have these situations when avoidable).

I think what we need to be able to do is to have a more general error type/container beginning at the NinaProtocolHandler struct impl here. And perhaps that's already an error type that we have, the main Error type defined in lib.rs.

So we could add a new Network classes of errors to this enum and even create a new sub type like ProtocolError called NetworkError. DnsResolveFailed could be a part of this new struct then. So the error stack looks like this then:

     Error(HighLevelType(InnerLevelType))
e.g. Error(Network(DnsResolveFailed))

This should be workable for all kinds of potential future classes of errors that we might want, even if we have a new HighLevelType and InnerLevelType.

Thoughts? Does my proposal make sense>

[calebbourg]

I think that makes sense and definitely fits the pattern we have so far.

Here is an example of a full error that I copied from our tests:

esp32_wroom_rp::Error::Protocol(
  esp32_wroom_rp::protocol::ProtocolError::InvalidNumberOfParameters
)

I wonder if we could get to something like this:

// Our existing errors will look like this
esp32_wroom_rp::Error::Protocol(InvalidNumberOfParameters)

// and a new network level error will look like this
esp32_wroom_rp::Error::Network(DnsResolveFailed)

It's essentially the same thing. Variants of our top level Error enum that contain lower level errors that are more specific and descriptive.

I think that we could define the lower level errors and then use Rust's import/export utilities to remove some of the deep module nesting.

For example maybe the network errors have a full path of:

esp32_wroom_rp::network::NetworkError::DnsResolveFailed

but are re-exported in such a way as to look like this to consumers of the crate:

esp32_wroom_rp::Error::Network(DnsResolveFailed)

I'm not sure exactly how to achieve that at the moment but I will try to play around and see if I can come up with something that feels right.

[calebbourg]

@jhodapp 8626cd0 is a start.

I added a network module that only contains a NetworkError enum. I also provided a From implementation which then allows us to use into() in our protocol level functions to smooth over the interface and only ever return top level Errors (except when we return Infallible.

[jhodapp]

This looks great, it turned out to be pretty straightforward with Rust to export the NetworkError symbol where we want it in a logical place. I think network makes a lot of sense as a module name that we can use for other things as well. For example, I think moving our simple IpAddress type into this module makes a lot of sense as well.

For anyone else following this conversation and for future reference, here's where NetworkError was added via Caleb's PR.

[calebbourg]

Moving IpAddress is a great idea!