Add support for register blocks

[alexmoon]

I have a fairly complicated I2C device I'm building a driver for. It has several blocks of registers that are repeated at different offsets, as well as blocks of registers that are logical units. I wanted to be able to define registers in blocks that could optionally be offset by a base address.

This does introduce a couple of breaking changes:

• The AddressType of a register device is abstracted out to a super-trait: AddressableDevice
• The write_register and read_register methods no longer have a generic parameter for the register type. Instead they take the address as a parameter to the function, so that it can be offset by a child RegisterBlock when it calls up to the parent implementation.

[diondokter]

Thanks!

Yeah this is something that was still missing.
But is your PR done? I don't see RegisterBlock being impl'd anywhere so I'm not really sure how it's supposed to be used.
There also doesn't seem to be any parsing and generation for repeated registers...

Passing the addresses as an argument instead of via constant in the register is fine by me. Should actually make binary size a bit better too.

[diondokter]

Thinking about it, repeated registers should probably get a repeat count and a stride.
Then instead of doing device.my_register().read(), you'd have to do device.my_register(12).read().

Looking at what is I think the device you're talking about: https://www.azoteq.com/images/stories/pdf/iqs323_datasheet.pdf
(Sorry, stalked your github :P)
I think that would work for you too.

For example, you'd make a ChannelSetup register with a repeat count of 3 and a stride of 0x10.
Do the same for the other channel setup registers and now you can index into them with your channel number.

device.channel_touch_settings(2).read()

Or do you foresee problems?
Do you really need the block representation?

[alexmoon]

Yeah this is something that was still missing. But is your PR done? I don't see RegisterBlock being impl'd anywhere so I'm not really sure how it's supposed to be used. There also doesn't seem to be any parsing and generation for repeated registers...

Yeah, I can add an example to the test-macro-driver example. I wasn't sure exactly how you'd want to model the syntax for parsing and generation, and I didn't need it for my use so I didn't implement that side of things.

Thinking about it, repeated registers should probably get a repeat count and a stride. Then instead of doing device.my_register().read(), you'd have to do device.my_register(12).read().

Or do you foresee problems? Do you really need the block representation?

That would work for this device, but it feels like a less general solution to me. The block representation is also nice because it lets me break up the functionality of the device for clearer documentation and auto-complete. For example, I've made each group of registers in the IQS323 a separate register block, even if there's only one instance. So I can call device.sys_info().system_status().read() instead of having all the registers attached directly to the Iqs323 struct in one long list.

I've pushed a branch of my iqs323-driver that uses these register blocks here: https://github.com/tactile-eng/iqs323-driver/tree/new-device-driver

[diondokter]

I see your desire for the blocks, but I'm not in love with the trait.

See, the goal of this crate is to generate all definitions of the registers. Having a trait that requires you to do manage manual address offsets doesn't fit that goal. So I'm inclined to say that either we generate blocks in the macro or we don't do anything with blocks.

Now, you think this trait works well for you and that's great. And I think there's a way we can both be happy.
So let's keep address as an argument and the AddressableDevice, but not the RegisterBlock trait.
This makes it possible for you to define the trait in your own driver and use it as as you do now.

Do you think that'd work for you?

[alexmoon]

I don't think it'll be difficult to add support for register blocks to the macros. Would this solution work for you if I add support to the macros?

I can't really define the trait in my own driver, because the blanket impl is not allowed when it's not in the same crate as the RegisterDevice and AsyncRegisterDevice traits.

[diondokter]

I don't think it'll be difficult to add support for register blocks to the macros. Would this solution work for you if I add support to the macros?

Sure, if you want to put in the work. Do you have an idea on how to do it? In principle it's not that difficult no, but how do you take the inputs? Currently all registers in the json, yaml and rust macro are in the same 'namespace'.

So there are questions:

• How do you define blocks?
• Do we allow a global 'anonymous' block (so what we have right now)
• You need repeated blocks, so how should users specify that?
• There's more than just registers. Maybe you should be able to put buffers and commands in the same blocks with registers too.
  • If we allow that, then how do they interact with repeated blocks?

I can't really define the trait in my own driver, because the blanket impl is not allowed when it's not in the same crate as the RegisterDevice and AsyncRegisterDevice traits.

True, so maybe the trait itself won't work, but you're already using a macro and that could generate the code you need

[alexmoon]

Ok, I'll go ahead and split this PR into one with just the AddressableDevice trait and address argument and continue working on adding register blocks to the macros and generation. I think I've got solutions to all your open questions, but we'll see after I get into it.

[diondokter]

Cool! Feel free to open an issue to discuss your plans. Would be a waste of time if you made a fully worked out PR only for me to reject it because I didn't like the direction you took. But that's up to you :)

Let me know here when this PR is ready

[alexmoon]

Ok, here's what I've got. I haven't implemented the from_macro side yet, just the generation from Yaml/JSON. I've tried to make it very easy to define and use register blocks. Let me know what your thoughts are.

To answer your questions:

* How do you define blocks?

Define a register with block, block_description and registers fields instead of the typical rw_type, size_bits, byte_order, reset_value, and fields fields.

* Do we allow a global 'anonymous' block (so what we have right now)

Yes. I think for simple devices that makes the most sense and wouldn't want to lose it.

* You need repeated blocks, so how should users specify that?

Define a register with just a block field that matches the name of a block field in a register that provides the definition for the block.

* There's more than just registers. Maybe you should be able to put buffers and commands in the same blocks with registers too.
  
  * If we allow that, then how do they interact with repeated blocks?

I couldn't come up with a good answer for what it meant to have commands or buffers in repeated blocks, so I've made blocks exclusive to registers for now.

[diondokter]

Thanks for the work so far! Good to see you're able to find your way through the codebase that should probably be commented and documented a lot more :P

[alexmoon]

Ok, try this version on for size. I implemented the syntax you suggested, including the repeated register feature. There are a couple of minor oddities:

• The type returned by foo_1() is Foo0 which may be slightly confusing
• If you copy a block that is repeated, the copy is also repeated.

[diondokter]

Sweet! I did not expect you to implement the repeat. But this is looking really nice. Only a couple of small things.

I guess only thing left is adding these features to the rust macro variant.
Probably best to add a new keyword so it'll look like:

/// Block comment here
block Foo0 {
    const BASE_ADDRESS: u8 = 42;
    const REPEAT = { count: 3, stride: 2 };

    register Bla {
        // ...
    }
}

I think that'd be most consistent with what's already there.

Really appreciate the work man!

[diondokter]

Pretty happy with this state. Couple of small things and we can merge

[diondokter]

The way you set this is might have me refactor it later. I don't know yet. But for now it's fine. I'd have made block (and maybe ref) its own thing. If this needs to be extended in the future, it's gonna be a bit difficult. But the refactor can be done then as well, so let's just leave this be.

[diondokter]

I guess it reflects the generation crate setup. So the same thing might come up at some point there too

[diondokter]

Interesting.

At some point I might want to expand copy_of to single registers (and not full blocks) and who knows what else...

I'm thinking whether this syntax will get in the way at some point.
For example, when refing a register instead of a block, we'd probably want to have an ADDRESS const instead of BASE_ADDRESS.
Not having the name of the 'type' we're ref-ing will complicate things.

So just for forward compatibility, we should make it ref block Foo1 or block ref Foo1. Which one would you prefer? I think I like ref block best. It sets precedence for having other keywords in front, e.g. proto block which could be just the definition of a block without instantiation so that you can give the block types nice names. Idk... Not sure if that will ever be a thing.

Sorry for the rambling :P

[alexmoon]

I agree that copy_of should eventually be expanded to single registers, other refs, etc. But I don't think we should make the contents of a ref dependent on the type of thing being copied. Perhaps the syntax should be something like:
ref Foo1: Foo0 = 80;
or
ref Foo1 = Foo0 @ 80;

[diondokter]

I see what you're thinking. However, that's assuming that the only thing a copy_of ever changes is the address. At least for a register, one might want to change the readwrite permissions. This happens a lot where you have interrupt registers with status, set, clear, etc.

So while a bit more verbose, I like my suggestion better because it leaves more options open for the future

[alexmoon]

One thing to note here is that the generation already supports copying registers as well as blocks (though not overriding any of the register's properties). So right now you could do:

register Foo { ... },
ref Bar = Foo { const BASE_ADDRESS: u8 = 12; }

And it will compile. Adding the block keyword won't actually change anything in the generation so ref block Bar = Foo ... will still compile, which seems wrong. If you want to allow overriding properties in a "copy_of", that may require re-thinking how it is modeled and how generation works.

[alexmoon]

Never mind, I think I have a solution for the above. It will just take a bit of reworking.

[diondokter]

Oh, and adding to the changelog would also be nice. Forgot about that in the last two PRs

[diondokter]

Just got back from vacation. I'll take a better look soon, but at a glance it looked good

[diondokter]

Thanks! This is a massive amount of effort. Well done