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

Proper error handling #3

Open
hellow554 opened this issue Jun 11, 2019 · 8 comments
Open

Proper error handling #3

hellow554 opened this issue Jun 11, 2019 · 8 comments

Comments

@hellow554
Copy link

/// Also perform WiFi Geolocation if it is enabled. Return 0 if successful.
pub fn start_network_task() -> i32 {
console_print(b"start_network_task\n");
0
}

Immediatly as I saw the video in the medium post I shuddered.

Do proper error handling in Rust!

Don't return ints to indicate errors or failures, use Result<(), ErrorType> instead. This is just one place, but everywhere where you have those ghastly assert!(rc == 0) things, throw them away and burn them! Either use unwrap, or better expect.

@hellow554
Copy link
Author

More examples:

pub fn send_sensor_data(_val: *const SensorValue, _sensor_node: &'static CStr) -> i32 {
console_print(b"send_sensor_data\n");
0
}

pub fn start_sensor_listener() -> i32 {

extern fn read_temperature(sensor: SensorPtr, _arg: SensorArg, sensor_data: SensorDataPtr, sensor_type: SensorType) -> i32 {

(why is that one declared as extern although it's not pub?)

pub fn start_network_task() -> i32 {

pub fn send_sensor_data(_val: *const SensorValue, _sensor_node: &'static CStr) -> i32 {

and finally you can convert the integer error codes to an enum

pub const SYS_EOK : i32 = 0;
pub const SYS_ENOMEM : i32 = -1;
pub const SYS_EINVAL : i32 = -2;
pub const SYS_ETIMEOUT : i32 = -3;
pub const SYS_ENOENT : i32 = -4;
pub const SYS_EIO : i32 = -5;
pub const SYS_EAGAIN : i32 = -6;
pub const SYS_EACCES : i32 = -7;
pub const SYS_EBUSY : i32 = -8;
pub const SYS_ENODEV : i32 = -9;
pub const SYS_ERANGE : i32 = -10;
pub const SYS_EALREADY : i32 = -11;
pub const SYS_ENOTSUP : i32 = -12;
pub const SYS_EUNKNOWN : i32 = -13;
pub const SYS_EREMOTEIO : i32 = -14;
pub const SYS_EDONE : i32 = -15;
pub const SYS_EPERUSER : i32 = -65535;

@lupyuen
Copy link
Owner

lupyuen commented Jun 11, 2019

Hi Marcel: Thanks for the tip! The porting of the entire application from C to Rust is still in progress, once I'm done with the meaty parts (e.g. CoAP messaging), I'll come back to the return types. And write an article about this too!

Here's different pattern for returning results: https://rust-embedded.github.io/book/static-guarantees/design-contracts.html

impl<IN_MODE> GpioConfig<Enabled, Input, IN_MODE> {
    pub fn into_input_pull_down(self) -> GpioConfig<Enabled, Input, PulledLow> {
        ....

//  Pull down the GPIO pin
let pulled_low = input_pin.into_input_pull_down();

Any update operation on GpioConfig returns another GpioConfig object. It's like a chainable API.

Wonder if you have any thoughts on this approach?

@lupyuen
Copy link
Owner

lupyuen commented Jun 11, 2019

Also what do you think about error-chain?

http://brson.github.io/2016/11/30/starting-with-error-chain

@hellow554
Copy link
Author

Here's different pattern for returning results

No, that's not returning a Result (as you can see by the return type). It's just returning a struct.

Also what do you think about error-chain?

http://brson.github.io/2016/11/30/starting-with-error-chain

IIRC error-chain is no longer recommended for new designs. You can of course roll your own Error enum, which should be no problem at all. You don't need a framework. You don't get any real benefits from it when working on an embedded platform.

@lupyuen
Copy link
Owner

lupyuen commented Jun 11, 2019

Thanks Marcel, I'm testing expect() like this...

pub fn start_network_task() -> Result<(), i32>  {  //  Returns an error code upon error.
    Ok(())
}

    //  In main()
    let rc = start_network_task();  
    rc.expect("");

When I did this, the compiled ROM size bloated from 55 KB to 58 KB. I suspect this could be due to core::fmt, I need to check.

Any idea how I can cut this bloat?

@hellow554
Copy link
Author

I hope you are compiling with --release.

expect("") should be expect("start_network_task failed") or similar.

https://docs.rust-embedded.org/book/start/panicking.html could help

@lupyuen
Copy link
Owner

lupyuen commented Jun 11, 2019

Hmmm wonder why Result and expect() would cause this bloat compared with returning ints. Aren't they functionally equivalent?

That's one of the reasons I didn't include this for the article. Needs more investigation. For embedded systems, this kind of unexplained bloat could be showstoppers.

Thanks for your help! :-)

@hellow554
Copy link
Author

Personally, I would not avoid using a Result even if that means, that by binary increases by a few kb.

warning: unused std::result::Result that must be used

That's the most amazing thing! If you forget to use the Result, the compiler will tell you. When you return an int, you don't need to handle it and you can easily swallow them. Also it doesn't tell you right away, what happened. It's just a convention that 0 means OK and any other values means Error. With Result you get it with the type system. Amazing!

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

No branches or pull requests

2 participants