PoC bit_modify operations

[Szpadel]

To solve issue described in #15, I implemented simple PoC adding that functionality.

I'm not familiar with architecture here, so it might not be ideal, but looks like it works as expected.

let me know what do you think about it and I'll try to resolve issues or you are free to modify this PR yourself.

[diondokter]

Hey, thanks for the PR!

I don't have time today to look at it thoroughly, but I'll try tomorrow after work.

Impressive how you managed to get something done in the macro jungle 😅 I really need to move everything to a proc macro...

Anyways, just to make clear to me what you're trying to do:

• In your hardware interface, you want not only the value of the register that is to be written, you also want a mask of the bits that have been written to.
• Those bits should be set to 1.
• Any bit that has been written to should be 1, even when the value remained the same.

Can you please tell me if these points are true?

[Szpadel]

Yes, all points are true,

Just to clarify, not all write operations should work like that, as this is required only for some registers (like mentioned in issue irq ack).

One thing that I didn't account in this is that not all registers support bit modify operation, but I wasn't sure how to accomplish that.

Ideally we would need optional trait implemented for interface to do bit modify operation and we would probably want to generate new register type for that (we have R and W, so maybe B?) instead of adding this to W.

If I'm missed some detail you might check my exact use case here: https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2515-Stand-Alone-CAN-Controller-with-SPI-20001801J.pdf section 12.10 explains bit modify operation

[diondokter]

Ok, so a couple of things.

So, I really don't have the time to make this myself. I'm in the middle of some construction in my house and that takes up pretty much all of my spare time. But I can review and will respond if you continue.

I would like to keep this separate of the W. I think it'd be pretty wasteful to make the W twice as big for a feature that won't be used a lot. I'd also like it to be a bit more generic, so no default implementation on the hardware. Just give a bitmask and the value. That's way more useful for most people. You can then send them in the proper order to your device in your hardware implementation in your driver. I also don't really like the name bit modify, but I don't have a better one. Maybe just modify? Because it's all about bits anyways?

If the name is modify, then we can use the m letter to indicate it.

But here it starts adding complexity real fast. It'd be best to create new register-level access modifiers. Next to RO, WO & RW, you'd also get MO & RM. Does that sound logical to you?

Sorry I can't be of more help. I want to put some more time into this crate, but I just don't have it right now.

[Szpadel]

I'll try to continue work on this in my sparse spare time (newborn).

I agree with creating separate struct for keeping value with mask, I wanted to do that from start, but I didn't got how, so I need to experiment with this lib more.

I didn't include any hardware specific implementation, there is just another trait to implement, only "hardware" in this PR is in example.

I also wanted to use modify at first, but the issue is that this is already used for W, so as far as I understand this name cannot be used twice. IMO bit_modify is fairly self explanatory what motivation is here.

I'll try to add register level modifiers, but I'm not sure it that's best way to go, as those modifiers are on bit level and not register level, so how this should work if some bits are eg RW and some RM?

[Szpadel]

I reimplemented this feature, now there is dedicated struct for bit modify operation (B form Bit - IMO it's better to indicate that those are bit operations instead Modify, as there is already modify operation that work in read-set-write pattern) and you need to declare register as RWB to get access to that operation. All is optional and require additional trait to be implemented, therefore this should be fully backward compatible change.

There is only write part in bit_modify as I do not feel that read does not really makes sense here, as the whole point is to eliminate race conditions and reading value before could lead to creating one at different level.

Previous implementation was just as PoC, now I am fairly happy with this interface. Maybe there should be some more documentation about that, but I'm not feel fluent with those macros enough to be comfortable doing.

[diondokter]

Cool! Just glancing at it I can see that it looks pretty decent. I'll try to find some time to do a proper review soon :)