Don't add is_VARIANT when VARIANT is 'set' or 'clear' and width is 1

[adamgreig]

If a field enumeratedValues includes variants set or clear, and the field width is 1, then the generated Rust contains a duplicate is_set or is_clear method.

I think it's reasonable for an SVD file to have a Clear variant, in the verb sense, for clearing an interrupt flag in a Flag Clear Register. It gives you a clear() method on the field, rather than bit(false).

This patch skips generating is_VARIANT when this condition is met.

[japaric]

This sounds good to me in principle but I'd hate if this introduced a bug because a vendor decide to map, for example, SET to the value 0. Could you add an assertion to check that if the vendor used CLEAR / SET for the enumeratedValue name then its value must be 0 / 1?

[adamgreig]

About that...

[japaric]

I knew this would happen.

I think it'd be better to rename the bit-level set, clear, is_set, is_clear methods so they don't become ambiguous with the variant-level methods. Hmm, maybe ... set_bit, clear_bit, bit_is_set and bit_is_clear? At least the last two can collide with the is_$variant methods. Of course a vendor could name a variant SET_BIT or BIT_IS_SET but I think the best we can do is reduce the chance of name collisions.

cc @whitequark #92

[whitequark]

@japaric seems really ugly but I have no better solution.

[japaric]

Another way I just thought is ... using another proxy struct. so you can do $bitfield().bit().set() for the bit-level method and $bitfield().$variant() for the enumeratedValue API. However, the vendor could name an enumeratedValue BIT and that would collide with the bit method that returns the proxy ...

[whitequark]

I don't think collisions are an issue, we could simply deterministically rename anything that can collide. I.e.: SVD bit -> Rust bit_, SVD bit_ -> Rust bit__ etc.

[adamgreig]

I think svd2rust already skips automatic set and clear methods if an enum is provided, so you could do the same for is_set and is_clear (the opposite of this patch, which prevents is_VARIANT instead). Slight saving grace is that these particular fields are WO so you'll never be calling is_clear anyway, and that's probably true of most fields where the clear verb means "write a 1". It does mean you'd never actually generate a collision.

The confusion is you can't tell from reading the code if hifcr().modify(|w| w.cfeif7().clear()) is setting the bit to zero or clearing the flag... but it's a shame to lose the short and nice set() and clear() for undocumented single-bit fields, of which there are so many.

It's a pity there's no obvious way to have the proxies take an argument directly, like w.cfeif7(true).cfeif8(true) for the automatic single bit cases, but I guess you can't then upgrade to an enumerated description later. Perhaps set_bit() and clear_bit() are best, and just hope to get good enums for as many fields as quickly as possible?

[japaric]

I think svd2rust already skips automatic set and clear methods if an enum is provided

Err, does it do that? Because it shouldn't. Adding enumeratedValues information should not break existing code.

Perhaps set_bit() and clear_bit() are best

Either this or the bit proxy works for me.

[adamgreig]

I think so... Starting here:
https://github.com/japaric/svd2rust/blob/master/src/generate.rs#L1054-L1063
it then generates methods for the variants if they are specified, otherwise:
https://github.com/japaric/svd2rust/blob/master/src/generate.rs#L1242-L1256

For example https://docs.rs/stm32f30x/0.4.1/stm32f30x/gpioa/bsrr/struct._Br11W.html has a reset(), variant(), and bits methods, but no set() or clear().

[japaric]

I think so... Starting here:

Ugh, I'll fix that.

@adamgreig Want to try to implement the bit proxy plus @whitequark's renaming idea. We can probably skip the renaming for now until it becomes a problem in practice.

[japaric]

Assuming that both of you are on board with the bit proxy idea.

[adamgreig]

How about .bit(true) and .bit(false) in line with .bits(1), rather than .bit().set() and .bit().clear()? Has the same potential for collision (with the same renaming solution) but without needing an extra proxy object.

Edit: that's only helpful in the write direction, of course. For reading, you could have .bit() return bool, but I think being able to check is_set() or is_clear() reads better, so perhaps the proxy object is the best bet.

[japaric]

I'd prefer to keep the set and clear methods for the write operation. They can be more readable than bit(bool) in some cases.

[whitequark]

@japaric I would prefer set_bit and clear_bit, there is already way too much nesting in svd2rust output.

[adamgreig]

👍 for set_bit() rather than further nesting. I'll do those and the conflicting-variant-renaming in a separate PR tonight. How does bit_set() compare to set_bit() and etc?

[whitequark]

Elsewhere svd2rust goes for English word order, so set_bit seems preferable.