A rough idea of a code structure

[george-cosma]

This PR is in responses to #3 and an outline to what was discussed there. It contains no implementation details.

Perhaps, it would be worthwhile to have an additional enum that would contain the different types of interfaces so that we can abstract away what type of interface we actually have.

enum Interface {
    Serial(SerialInterface),
    OpenOCD(OpenOCDInterface),
    JLink(JLinkInterface)
}

fn create_interface(cli_args: ...) -> Interface {
    ...
}

But then, it would be a bit inconvenient to then call any type of function defined in BoardInterface, and the whole concept of BoardInterface and Interface kinda overlap as a concept.

let interface = create_interface(cli_args);

interface.open(); // doesn't work as Interface does not implement 'BoardInterface'

match interface {
    Interface::Serial(serial) => serial.open();
    ...
}

As an alternative, we could hide the whole match call into an implementation of BoardInterface for Intreface. Possibly, a macro could also be helpful here.

We could then again forget about the enum altogether and just use function parameters that implement the BoardInterface trait:

fn listen(interface: &impl BoardInterface) {
    ....
}

But this way we need different code paths for all the the interfaces:

if cli_args.has_openocd_flag() {
    listen(create_openocd(cli_args));
} else ...

Suggestions?

[george-cosma]

After some research, I've realized enum_dispatch could be useful for us. I'm still a bit unsure about the naming, BoardInterface and Interface names are quite similar and overlap in their purpose. More so, Interface is quite a generic name.

[valexandru]

The code structure looks ok, but I don't really understand the need for the enum_dispatch.
Otherwise, on the long term it might be a good idea to separate the traits (such as board_interface) and the implementations (such as jlink) if many trait instances will appear.

[george-cosma]

enum_dispatch isn't needed, but it will minimize the amount of code written. Let me explain why I think so.

So, we have 3 interfaces (structs): JLinkInterface, OpenOCDInterface and SerialInterface, all of which implement the trait BoardInterface. Let's imagine how we could use them. We know we want to be able to interact with a board in any way the user desires - for example if I hook up my board to my PC, I can tell (python) tockloader specifically if I want to use OpenOCD or Serial, and the way I do it is by specifying --serial as a command line argument or not.

The rust-equivalent code would look something like this:

/* cli parsing */
if cli_args.has_flag("serial") {
   /* Initialize Serial Interface*/
} else {
   /* Initialize OpenOCD Interface*/
}

The problem now comes, how do we initialize them? We can't do

let interface = if cli_args.has_flag("serial") {
    OpenOCDInterface {port: 22} // "port" is just an example struct member. We could have a function here instead, idk
} else {
    SerialInterface {port: 22}
};

interface won't have a type known at compile-time. So we could use Box<dyn BoardInterface> to solve the issue:

fn get_interface(args: ...) -> Box<dyn BoardInterface> { /*...*/ }

let interface = get_interface(cli_args);
interface.open();

But this has two downsides:

1. Performance overhead. enum_dispatch explains a bit why that is.
2. Using a Box<dyn ...> is messy when passing is as an argument to other functions (which we might need to do)

fn get_data_from_interface(interface: &(impl BoardInterface + ?Sized)) -> u32 { /* ... */ }
// ...
let data = get_data_from_interface(interface.as_ref());

A (cleaner) alternative would be using an enum ( as I have in this PR).

pub enum Interface {
    Serial(SerialInterface),
    OpenOCD(OpenOCDInterface),
    JLink(JLinkInterface),
}

fn get_interface(args: ...) -> Interface { /*...*/ }
fn get_data_from_interface(interface: Interface) -> u32 { /* ... */ }

let interface = get_interface(cli_args);
let data = get_data_from_interface(interface);

The issues with this comes when we try to interact with the BoardInterface trait. Although all variants of this enum implement BoardInterface, Interface enum doesn't implement it. So doing interface.open() is not possible. We could then implement the trait on it:

impl BoardInterface for Interface {
    fn open(&mut self) -> Result<(), TockloaderError> {
        match self {
          Interface::Serial(a) => a.open(),
          Interface::OpenOCD(b) => b.open(),
          Interface::JLink(c) => c.open(),
        }
    }
}

This isn't that bad, it's quite readable code. The issue comes when we have many traits with many defined methods.
enum_dispatch is a programmatic macro crate which takes care of this code repetition.

Personally, I think enum_dispatch is the best option out of the ones I have presented (and that I know of).

[valexandru]

It makes sense. Thanks for the explanation. The only thing that remains on long term is splitting the traits and the implementations into separate folders if needed.

[george-cosma]

It makes sense. Thanks for the explanation. The only thing that remains on long term is splitting the traits and the implementations into separate folders if needed.

The more I think about it, the more I agree with this idea. I've never been a fan of "monolith" files, with hundreds and hundreds of lines of code. My initial intention for "BoardInterface" was to be a mega-trait, but that just sounds like a bad idea now.

I've let this PR open for a while, and since no new comments were posted I'll go ahead and add today a mockup of how this split-up implementations might look.

[george-cosma]

@valexandru Is this (latest commit) what you had in mind?

[george-cosma]

I was thinking, maybe we should use anyhow::Error instead of TockloaderError. Or maybe both? After working with TockloaderError, it definitely felt a bit... useless, or extra code for little reason. Most of the error cases would be errors from other crates.

[george-cosma]

Okay, after some research I've narrowed down our possibilities of error-handling:

1. Keep going as is with std error

I'm not a fan of this, but it is the most lightweight version. Furthermore, if we do keep the tbf-parser library in here, their error-handling would be similar (as the other options may require the use of std, which we should avoid to maintain potential compatibility with the version on the tock repo.)

2. anyhow crate

Anyhow would be a simple solution. Whenever we get an error - just bail!() - we don't care what it is that went wrong or why it originated. Eventually, we give some context clues to the user via an error message.

3. thiserror crate

This would make the creation of errors a bit nicer, but then again a non-trivial effort. Not to mention if anyone not used to rust were to come work on the project, they might be required to introduce a new error. Learning a bit of thiserror might be an extra step, although not a huge one.

4. Box <dyn Error>

It works well, and the performance hit shouldn't be that bad for any computer made after 2005. Though a bit ugly.

Reports?

Rust does have a new, experimental, feature called Reports. It's supposed to work in conjunction with the regular errors as opposed to a replacement. I'm not sure how useful it would be to us since we won't really have nested errors (or at least not deep enough for a error trace to make much sense). The additional costs in maintaining it aren't worth the advantages it brings, but I did consider worth mentioning here, just to bring it into discussion.

My suggestion

I would personally opt to use thiserror. While anyhow might be the most logical choice, we have binary app, and we want to return error messages to the user, not another application, I still believe that a tight type system feels more rust-like. Although I am not certain that this is actually the best option. I could easily see the following scenario repeat again and again: adding a variant which would only be used in a single place. Unless a non-trivial amount of time is put into the design of TockloaderError to have generic, multi-use variants, I could certainly see hyper-specific enum variants pop up every other pull request. Not amazing.

Any suggestions?

P.S. Here is how an implementation of an error might look like using thiserror and anyhow.

/****** THISEROR + ANYHOW *****/

use thiserror::Error;

#[derive(Error, Debug)]
enum DivisionError {
    #[error("Divisor can't be zero")]
    DivisionByZero,
    #[error("Divisor ({0}) can't be negative")]
    DivisionByNegativeNumber(i64),
}

// Note: the backtrace will not enter this function.
fn divide_shorter_backtrace(dividend: i64, divisor: i64) -> Result<i64, DivisionError> {
    if divisor == 0 {
        Err(DivisionError::DivisionByZero.into())
    } else if divisor < 0 {
        Err(DivisionError::DivisionByNegativeNumber(divisor).into())
    } else {
        Ok(dividend / divisor)
    }
}

fn divide(dividend: i64, divisor: i64) -> Result<i64, anyhow::Error> {
    if divisor == 0 {
        Err(DivisionError::DivisionByZero.into())
    } else if divisor < 0 {
        Err(DivisionError::DivisionByNegativeNumber(divisor).into())
    } else {
        Ok(dividend / divisor)
    }
}

// RUST_BACKTRACE=1 cargo run
// will print the backtrace
fn main() -> Result<(), anyhow::Error> {
    println!("Hello, world!");

    divide(10, 2)?;
    divide(10, -2)?;
    divide(10, 0)?;

    Ok(())
}

[bradjc]

Some comments, but overall this makes sense to me.

[bradjc]

While the file in tockloader is called board_interface.py (and has been for a long time), in the tockloader.py code I've switched to effectively calling this a "channel". So perhaps renaming this to Channel or BoardChannel might resolve some of the headaches with overloading the word "interface".

[george-cosma]

Yep, I 100% agree with you on this one. I like the name Channel way better. The name of the Channel enum and BoardChannel trait still kind of overlap each other, but that's mostly because I don't know yet how I want to structure the traits, and what behaviour is common between the channels.

[bradjc]

I don't think each interface/channel needs its own folder, that seems like unnecessary complexity to me. Each interface/channel should be not too complicated, in the hopes that adding new ones isn't too difficult. They should all have the same functionality as well, again suggesting they should be somewhat simple.

That being said, I don't feel strongly about this.

[george-cosma]

So you're thinking that each trait should be implemented for all variants (Serial/JLink/OpenOCD) in the same file? Both this, and the current options are equal in my opinion. Though one advantage could be that we could move the declaration to said file (In effect having both the declaration and implementations in the same file). This could prevent trait.rs from becoming overtly bloated. The traits could optionally be re-exported somewhere?

[bradjc]

No, separate files would still be good. Even the folders isn't really a problem. I just think we would like to minimize the amount of per-channel code that is required or expected, because we want to minimize the amount of effort to add a new channel (ie esp tool).

[george-cosma]

After returning from the holiday break, I'm no sure anymore I completely understand what you were suggestion. So, you are effectively suggesting going from:

📂channels
 ┣ 📂jlink
 ┃ ┗ 📜board_interface.rs
 ┣ 📂openocd
 ┃ ┗ 📜board_interface.rs
 ┣ 📂serial
 ┃ ┗ 📜board_interface.rs
 ┣ 📜jlink.rs
 ┣ 📜openocd.rs
 ┣ 📜serial.rs
 ┗ 📜traits.rs

to something like this?

📂channels
 ┣ 📂traits
 ┃ ┗ 📜board_channel.rs <- contains impl blocks for all channels
 ┣ 📜jlink.rs
 ┣ 📜openocd.rs
 ┣ 📜serial.rs
 ┗ 📜traits.rs <- still contains the definition of traits