Rethinking this crate and structure

[diondokter]

We're running into some problems:

• DLS parsing is overly strict
  • Try changing the order of the consts and types
• User errors can be better
• Processing of data is done in many different places and needs streamlining
• Some features are implemented in some cases and are missing in other
• Commands need an overhaul to be able to carry data and return values

I think all these issues stem from the fact that this crate is really a compiler, but isn't structured as a compiler. It doesn't help that this was my first big proc macro I wrote.

So the solution? Make it look more like a compiler. Something like this:

                                                       Close to textual/           Check for correct         Fully formed types         Straighten data            The exact, correct                                                                                      
                                                       token input with            keywords in right         but not yet type           to make it easy to         data we're gonna                                                                                        
                                                       little validation           places and no             checked and things         generate output            generate code from                                                                                      
                                                                                   duplicates                may be wrong               from                                                                                                                               

┌────────────────────┐     ┌────────────────────┐     ┌────────────────────┐     ┌────────────────────┐     ┌────────────────────┐     ┌────────────────────┐     ┌────────────────────┐     ┌────────────────────┐     ┌────────────────────┐                             
│                    │     │                    │     │                    │     │                    │     │                    │     │                    │     │                    │     │                    │     │                    │                             
│ DSL                │     │ Parsing with       │     │ Syn-based HIR      │     │ Syntax             │     │ MIR                │     │ Semantics checking │     │ LIR                │     │ 'quote' generation │     │ Tokenstream        │                             
│ Macro              ├─────► Syn                ├─────►                    ├─────► Checking           ├─────►                    ├─────► and resolving links├─────►                    ├─────►                    ├─────► Output             │                             
│ Tokens             │     │                    │     │                    │     │                    │     │                    │     │                    │     │                    │     │                    │     │                    │                             
└────────────────────┘     └────────────────────┘     └────────────────────┘     └────────────────────┘     └─────────▲──────────┘     └────────────────────┘     └────────────────────┘     └────────────────────┘     └────────────────────┘                             
                                                                                                                      │                                                                                                                                                    
┌────────────────────┐     ┌────────────────────┐     ┌────────────────────┐     ┌────────────────────┐               │                                                                                                                                                    
│                    │     │                    │     │                    │     │                    │               │                                                                                                                                                    
│ Json               │     │ Parsing with       │     │ Serde-based HIR    │     │ Syntax             │               │                                                                                                                                                    
│ Text               ├─────► serde-json         ├─────►                    ├─────► Checking           ├───────────────┘                                                                                                                                                    
│                    │     │                    │     │                    │     │                    │                                                                                                                                                                    
└────────────────────┘     └────────────────────┘     │                    │     └────────────────────┘                                                                                                                                                                    
                                                      │                    │                                                                                                                                                                                               
┌────────────────────┐     ┌────────────────────┐     │                    │                                                                                                                                                                                               
│                    │     │                    │     │                    │                                                                                                                                                                                               
│ Yaml               │     │ Parsing with       │     │                    │                                                                                                                                                                                               
│ Text               ├─────► serde-yaml         ├─────►                    │                                                                                                                                                                                               
│                    │     │                    │     │                    │                                                                                                                                                                                               
└────────────────────┘     └────────────────────┘     └────────────────────┘                                                                                                                                                                                               

Maybe the two HIRs can be collapsed into one? I don't know.

This should hopefully unlock more easy options for building new features and sharing features between different parts.

One other reason for this change is that the commands need expanding. Commands right now are just a single command. It doesn't carry data and it can't return anything. It would be awesome if we could reuse the field logic of the registers.

Blocks right now cannot carry commands or buffers or other blocks.

Registers cannot be repeated now.

So many things...

[diondokter]

DSL grammar and HIR implementation can be found here: https://github.com/diondokter/device-driver/blob/compilify/generation/src/dsl_hir.rs

[felipebalbi]

1. strict_conversion for everything (probably through EnumVariant above)
2. replicate bilge's #[fallback]
3. replicate bitfield-struct's with_* APIs
4. replicate svd2rust's read(), write(), and modify() APIs.
5. Maybe add a "generic" Transport trait which could be spi, i2c, uart, mmio, anything.
6. Maybe add a generic "register cache" (probably behind a feature) such that we can reduce the amount of reads from a register if we are certain a register will not change without our intervention.

[diondokter]

Thanks for the feedback @felipebalbi!

I do have some questions/remarks:

1. strict_conversion for everything (probably through EnumVariant above)

Do you mean not having any 'loose' conversions? Or just changing the default? With the extra possible validation steps, we could check if there's a variant for every possible value and return an error if not everything is covered. So the strict option would become more usable.

2. replicate bilge's #[fallback]

That's what the 'default' option is for the enums. I'm using num-enum for all this and it has a 'catch-all' too which is the fallback with number. I guess I could expose that. Also, thinking about it, since I'm already generating code, I don't really need num-enum. I could just generate that code directly.

3. replicate bitfield-struct's with_* APIs

The current API is already kinda like this. I don't see why an explicit builder pattern is needed. You can already do: foo.write(|w| w.bar(12).ree(true))

4. replicate svd2rust's read(), write(), and modify() APIs.

What do you mean? It's already like that. Or do you want an explicit R value in the modify?

5. Maybe add a "generic" Transport trait which could be spi, i2c, uart, mmio, anything.

I'm unsure what you mean. How's that different to the existing trait? https://docs.rs/device-driver/0.6.0/device_driver/trait.RegisterDevice.html

6. Maybe add a generic "register cache" (probably behind a feature) such that we can reduce the amount of reads from a register if we are certain a register will not change without our intervention.

Good idea! Though a feature flag is probably not the right method since that would affect all drivers in the dependency tree made with this. But making it part of the global config could be done. OTOH, users can already implement this themselves now, so it's not high-prio to implement.

[felipebalbi]

Thanks for the feedback @felipebalbi!

I do have some questions/remarks:

1. strict_conversion for everything (probably through EnumVariant above)

Do you mean not having any 'loose' conversions? Or just changing the default? With the extra possible validation steps, we could check if there's a variant for every possible value and return an error if not everything is covered. So the strict option would become more usable.

depending on the size of the bitfield, it might be impractical to define enums for all variants, but in general, yeah. Even for single-bit fields. For example, I'm writing a driver for a battery charger IC where some bitfields Enable is 1, while in others Enable is 0. Rather than forcing the user to remember that an enum helps considerably.

2. replicate bilge's #[fallback]

That's what the 'default' option is for the enums. I'm using num-enum for all this and it has a 'catch-all' too which is the fallback with number.

oh, I see. I missed that. Thanks for the info.

I guess I could expose that. Also, thinking about it, since I'm already generating code, I don't really need num-enum. I could just generate that code directly.

Sounds like a good idea. Less dependencies 💯

3. replicate bitfield-struct's with_* APIs

The current API is already kinda like this. I don't see why an explicit builder pattern is needed. You can already do: foo.write(|w| w.bar(12).ree(true))

Interesting. Seems like I missed this one too.

4. replicate svd2rust's read(), write(), and modify() APIs.

What do you mean? It's already like that. Or do you want an explicit R value in the modify?

I was more concerned about only have read() for readable registers, write() for writeable registers, and so on. Currently, RegisterDevice always requires write_register() and read_register() to be implemented. Additionally, for the bitfields themselves, svd2rust also handles the "special cases" one-to-set, one-to-clear, zero-to-set, zero-to-clear, one-to-toggle, zero-to-toggle. This might come in handy for device-driver too.

5. Maybe add a "generic" Transport trait which could be spi, i2c, uart, mmio, anything.

I'm unsure what you mean. How's that different to the existing trait? https://docs.rs/device-driver/0.6.0/device_driver/trait.RegisterDevice.html

I mean something like this:

trait Transport {
    type Output;
    type Error;

    fn read(&mut self, addr: &[u8]) -> Result<Self::Output, Self::Error>;
    fn write(&mut self, addr: &[u8], value: &[u8]) -> Result<Self::Output, Self::Error>;
}

impl<I2C: I2c> Transport for MyDevice<I2C> {
    type Output = u16;
    type Error = I2C::Error;

    fn read(&mut self, addr: &[u8]) -> Result<Self::Output, Self::Error> {
    self.i2c.write_read(self.device_addr, addr)
    }

    fn write(&mut self, addr: &[u8], value: &[u8]) -> Result<Self::Output, Self::Error> {
    self.i2c.write(self.device_addr, addr, value)
    }
}

impl<SPI: Spi> Transport for MyDevice<SPI> {
    type Output = u16;
    type Error = SPI::Error;

    fn read(&mut self, addr: &[u8]) -> Result<Self::Output, Self::Error> {
    self.spi.write_read(self.device_addr, addr)
    }

    fn write(&mut self, addr: &[u8], value: &[u8]) -> Result<Self::Output, Self::Error> {
    self.spi.write(self.device_addr, addr, value)
    }
}

From the device driver perspective, the underlying bus (i2c, spi, uart, light pulses heh) is irrelevant as long as we can implement read and write moethods.

6. Maybe add a generic "register cache" (probably behind a feature) such that we can reduce the amount of reads from a register if we are certain a register will not change without our intervention.

Good idea! Though a feature flag is probably not the right method since that would affect all drivers in the dependency tree made with this. But making it part of the global config could be done. OTOH, users can already implement this themselves now, so it's not high-prio to implement.

understood

[alexmoon]

That's what the 'default' option is for the enums. I'm using num-enum for all this and it has a 'catch-all' too which is the fallback with number. I guess I could expose that. Also, thinking about it, since I'm already generating code, I don't really need num-enum. I could just generate that code directly.

One small issue with treating "default" as a value for enums is that it doesn't allow you to make a non-consecutive value the default. If the DSL could treat default as an attribute instead of a value it would allow enums like:

enum Foo {
    A = 0,
    B = 1,
    C = 2,
    #[default]
    Unknown = 0xff,
}

[diondokter]

One small issue with treating "default" as a value for enums is that it doesn't allow you to make a non-consecutive value the default. If the DSL could treat default as an attribute instead of a value it would allow enums like:

enum Foo {
    A = 0,
    B = 1,
    C = 2,
    #[default]
    Unknown = 0xff,
}

Hmmm, I see your point...

I've not yet found this to be a problem. But I'll think about it

[diondokter]

Btw, I started a branch with my work here: https://github.com/diondokter/device-driver/tree/compilify

[tulku]

Not sure if it is even doable, or relevant to this discussion, but I was now using the BufferDevice to implement a driver for an SPI RAM memory (https://www.issi.com/WW/pdf/IS62-65WVS2568GALL-BLL.pdf)

I want to store an array of equally sized objects in the memory, each element being a serialized struct.

I was wondering if it's possible to define the buffers as an array of elements (so that we can index them, or iterate through them).

I am now using this create https://crates.io/crates/seq-macro to create the buffers, to create buffers with ID==starting address, but I won't be able to iterate them, nor have the buffers be determined by the size of the serialized struct over the size of the memory (not sure if that is possible though, because of when macros get expanded).

[diondokter]

Not sure if it is even doable, or relevant to this discussion, but I was now using the BufferDevice to implement a driver for an SPI RAM memory (https://www.issi.com/WW/pdf/IS62-65WVS2568GALL-BLL.pdf)

I want to store an array of equally sized objects in the memory, each element being a serialized struct.

I was wondering if it's possible to define the buffers as an array of elements (so that we can index them, or iterate through them).

I am now using this create https://crates.io/crates/seq-macro to create the buffers, to create buffers with ID==starting address, but I won't be able to iterate them, nor have the buffers be determined by the size of the serialized struct over the size of the memory (not sure if that is possible though, because of when macros get expanded).

The buffers (at least right now) are a relatively thin layer on top of the embedded-io traits like: https://docs.rs/embedded-io/latest/embedded_io/trait.Write.html

I'm not fully sure what you're trying to do though... You want buffers that are not bytes? How's that relevant to a SPI RAM driver?

[tulku]

The buffers (at least right now) are a relatively thin layer on top of the embedded-io traits like: https://docs.rs/embedded-io/latest/embedded_io/trait.Write.html

That's interesting. I'll take a look there, on a first glance it might be a better interface to use for this application. Thanks!

I'm not fully sure what you're trying to do though... You want buffers that are not bytes? How's that relevant to a SPI RAM driver?

Ideally, I'd want to create an array of structs and store it in the SPI RAM "transparently". However, I am happy to have pre-defined N buffers (for each array element) of a size where the struct can be stored and use the buffers to read/write these structs as needed.

[diondokter]

Ideally, I'd want to create an array of structs and store it in the SPI RAM "transparently". However, I am happy to have pre-defined N buffers (for each array element) of a size where the struct can be stored and use the buffers to read/write these structs as needed.

Hmmm ok. I don't know your application. I'll just give the tip to keep separation of low level and high level concepts. I'd build a driver for the RAM where you'd be able to store bytes in locations. Then on top of that you can build a layer to store objects into any collection of bytes.

In any case, this issue is not the place to dicuss this. If you think your usecase is something this crate should support, then please open a new issue!

[diondokter]

Extra suggestion: Add defmt support

[diondokter]

Open question: We're using bitvec as the underlying crate for storing and getting bits. But it doesn't always do the obvious:

    #[test]
    fn bitvec() {
        let mut arr_lsb = bitarr![u8, Lsb0; 0; 16];
        let mut arr_msb = bitarr![u8, Msb0; 0; 16];

        arr_lsb.set(0, true);
        arr_msb.set(0, true);

        println!("LSB: {:X?}", arr_lsb.as_raw_slice());
        println!("MSB: {:X?}", arr_msb.as_raw_slice());
    }

This prints:

LSB: [1, 0]
MSB: [80, 0]

This means that bit 0 is always stored in byte 0. This goes against what would seem obvious where bit 0 LSB would be [0, 0]. This can't really be changed though. So maybe just document? Also, what to do with the reset value? You can specify it as an array of bytes and as an integer. Should the array of bytes follow the way that bitvec does it or not? Should the integer be parsed LE or BE? Or same as setting of the register?

[alexmoon]

Yeah, it seems like the Lsb0/Msb0 types only affect bit ordering within an element, not between elements. In other words, it doesn't affect MSB/LSB byte ordering for multi-byte registers. I don't think I've seen a device where Msb0 bit ordering would actually make sense (though I'm sure there are some out there). The current semantics are correct for an Lsb0/little-endian multi-byte register. However for a big-endian multi-byte register you would probably still want Lsb0 but would need to swap the bytes around in your read_register/write_register impl.

This means that bit 0 is always stored in byte 0. This goes against what would seem obvious where bit 0 LSB would be [0, 0]. This can't really be changed though. So maybe just document? Also, what to do with the reset value? You can specify it as an array of bytes and as an integer. Should the array of bytes follow the way that bitvec does it or not? Should the integer be parsed LE or BE? Or same as setting of the register?

I think an array of bytes should be passed directly to bitvec to avoid surprises. I don't know of a way to handle integers properly without an additional parameter for LSB/MSB byte order. This was a problem for me in my driver, I had to define all default values as byte arrays to avoid endianness issues.

[diondokter]

@alexmoon yeah. There are so many orderings going on. I need to document them all and figure out what should all be configurable and what should be the defaults.

[diondokter]

So I was writing out a little doc about memory ordering in this crate and I've found a crucial missing detail. I think that may be why my s2-lp isn't working either.

Say you have these registers in the datasheet:	Name	Address	Bits	Value
SYNT3	05	7:5	PLL_CP_ISEL
		4	BS
		3:0	SYNT[27:24]
SYNT2	06	7:0	SYNT[23:16]
SYNT1	07	7:0	SYNT[15:8]
SYNT0	08	7:0	SYNT[7:0]

Then it seems fair to combine them into one register that is defined as:

register OutX {
    const ADDRESS = 0x05;
    const SIZE_BITS = 32;
    type ByteOrder = BE;

    synt: uint = 0..=27,
    bs: bool = 28,
    pll_cp_isel = 29..=31
}

The synt here will be loaded as BE since we read the registers from low address to high address. However, this does not work in its current form! See, what this does is it first takes the bitrange 0..=27, and then reads the value using BE. But the underlying bitvec is still little endian!

This is what was meant by putting the BE there:

     ____   _________   _________   _________   <- Selected by range 0..=27
pppb_ssss | ssss_ssss | ssss_ssss | ssss_ssss   <- Bits of the data
^       ^ | ^       ^ | ^       ^ | ^       ^
32      24| 23      16| 15      8 | 7       0

But this is what is actually going on:

_________   _________   _________        ____   <- Selected by range 0..=27
pppb_ssss | ssss_ssss | ssss_ssss | ssss_ssss   <- Bits of the data
^       ^ | ^       ^ | ^       ^ | ^       ^
7       0 | 15      8 | 23      16| 31      24

Man... This totally missed me. I need to straighten this out. But I'm not sure if I can still use bitvec since its internal representation is always LE. That'd be sad because it's such a nice crate to work with...

edit: I think I can still use bitvec.

• When I know the register is BE, then I need to reverse the full byte array first that was read from the device and then make it a bitvec.
• After that all loads and stores must be done with LE when reading or writing the fields.
• When the register needs to be written out to the device, the bytes are reversed again and then sent over.

It's not yet clear to me how/if bit ordering affects things... I think not

[diondokter]

Alright, made a document explaining memory with examples of some devices and how to figure out what the byte and bit order are: https://github.com/diondokter/device-driver/blob/ac069fdc942b104e097b36a336b16a726d08d0a1/memory.md