FactbirdHQ / atat

no_std crate for parsing AT commands
Apache License 2.0
115 stars 31 forks source link

Const generics & heapless v0.7 #81

Closed MathiasKoch closed 3 years ago

MathiasKoch commented 3 years ago

Fixes #90

fkohlgrueber commented 3 years ago

I'd like to update to heapless 0.7. Any objections to that? If not, I can prepare a PR.

MathiasKoch commented 3 years ago

I have a few commits to this locally, including 0.7.. i got a bit stuck on some associated const, but i think I have figured out a solution.. can push it tomorrow, and tag you for a review?

fkohlgrueber commented 3 years ago

Sure, sounds good!

MathiasKoch commented 3 years ago

@fkohlgrueber I just pushed my WIP, but i still have some issues.

My problem is around AtatCmd::CommandLen and turning that into a const generic, which is now required by heapless v0.7.

Issue is, that min_const_generics is not enough to use associated consts in generic consts spots. This will yield

error: generic parameters may not be used in const operations
  --> atat/src/traits.rs:84:37
   |
84 |     fn as_bytes(&self) -> Vec<u8, { Self::CommandLen::USIZE }>;
   |                                     ^^^^^^^^^^^^^^^^^^^^^^^ cannot perform const operation using `Self`
   |
   = note: type parameters may not be used in const expressions
   = help: use `#![feature(const_generics)]` and `#![feature(const_evaluatable_checked)]` to allow generic const expressions

I am not sure how to get around this? I can't even see a way around it if we introduce typenum for parts of it again?

Any inputs are valuable, as i would love to see it all use const generics!

fkohlgrueber commented 3 years ago

Oh, that's tough!

I'm not an expert regarding const generics, but I think this would really need full const_generics unfortunately.

I tried to find a workaround and the best thing I could come up with is making the AtatCmd generic over its length:

pub trait AtatCmd<const LEN: usize> {
    type Response: AtatResp;
    type Error: FromStr;

    fn as_bytes(&self) -> Vec<u8, LEN>;
    ...
}

This then requires adaptions in other places, e.g.:

// before
fn send<A: AtatCmd>(&mut self, cmd: &A) ...;
// after
fn send<A: AtatCmd<LEN>, const LEN: usize>(&mut self, cmd: &A) ...;

Implementation of AtatCmd can still be done only on a single length:

pub struct CloseConnection {}

impl AtatCmd<15> for CloseConnection {
    type Response = String<256>;
    type Error = GenericError;

    fn as_bytes(&self) -> Vec<u8, 15> {
        ...
    }
    ...
}

I haven't fully tried this / thought this through, but this might work. What do you think?

fkohlgrueber commented 3 years ago

I wasn't sure how using this would look like, so I set up a simple example:

pub trait AtatCmd<const N: usize> {
    fn as_bytes(&self) -> [u8; N];
}

/// Command implementing AtatCmd for a single LEN
struct MyCmd;

impl AtatCmd<15> for MyCmd {
    fn as_bytes(&self) -> [u8; 15] {
        [0; 15]
    }
}

/// Command implementing AtatCmd for two different LENs
struct MyCmd2;

impl AtatCmd<30> for MyCmd2 {
    fn as_bytes(&self) -> [u8; 30] {
        [1; 30]
    }
}

impl AtatCmd<100> for MyCmd2 {
    fn as_bytes(&self) -> [u8; 100] {
        [42; 100]
    }
}

fn send<A: AtatCmd<LEN>, const LEN: usize>(cmd: &A) {
    let bytes = cmd.as_bytes();
    println!("Bytes: {:?}", bytes);
}

fn main() {
    // MyCmd implements AtatCmd only for a single LEN, so send can be called 
    // just as usual
    let cmd = MyCmd;
    send(&cmd);

    // MyCmd2 implements AtatCmd for two different LENs, so we need to annotate 
    // the send function call to specify which implementation to use.
    let cmd2 = MyCmd2;
    // simply calling send like this will not compile ("type annotations needed"):
    // send(&cmd2); 
    // turbofish to the rescue:
    send::<_,30>(&cmd2);
    send::<_,100>(&cmd2);
}

This looks pretty good to me. Compared to having the length as an associated const, it's possible to implement AtatCmd for multiple lengths instead of just one. I'm not sure if there are use-cases for having multiple impls with different lengths for the same command, but it doesn't seem to hurt either. If there's only a single impl, the command type can be used in generic functions (e.g. send) just as before. If there are multiple implementations, we need to use turbofish in the call to specify which implementation to call. To me that seems like an acceptable solution until more const_generics are stabilized.

MathiasKoch commented 3 years ago

I think that could work. Let me try to implement it, and push the changes here, so we can have a look in a broader context.

Implementation incoming...

MathiasKoch commented 3 years ago

Only place i am a bit in doubt if this could work, is when introducing the helper trait of AtatLen while deriving.

It looks like this

pub trait AtatLen {
    const LEN: usize;
}

The issue i see is that the AtatCmd impl would become

impl AtatCmd<<Self as AtatLen>::LEN> for MyCmd2 {
    fn as_bytes(&self) -> Vec<u8, <Self as AtatLen>::LEN> {
        ...
    }
}

Not sure if that works?

MathiasKoch commented 3 years ago

Hmm, got close :/

impl AtatCmd<{ <Self as AtatLen>::LEN }> for MyCmd2 {
    fn as_bytes(&self) -> Vec<u8, { <Self as AtatLen>::LEN }> {
        ...
    }
}

With the result generic 'Self' types are currently not permitted in anonymous constants :/

fkohlgrueber commented 3 years ago

Ah, so this doesn't work for things like Vec<T, L> or String<L>. I currently have no idea how this could be fixed then.

MathiasKoch commented 3 years ago

I think we have worked out a fix ;) Hang tight! :+1:

MathiasKoch commented 3 years ago

I just pushed a fix.

Would you give it a review @fkohlgrueber ?

I think the only limitation of this way of doing it is that #[derive(AtatCmd)] cannot be used on structs with generic types. Instead they have to be manually implemented.