svd2rust

svd2rust copied to clipboard

The way W1C (write-1-to-clear) fields are handled is counterintuitive. Basically, they're not handled differently from normal fields, which makes them counterintuitive from a programmer's perspective.

Suppose you have a register with a mix of RW and RW1C fields, like so:

Offset | Field    | Access | Description
-------|----------|--------|-------------
7      | irq_stat | RW1C   | 1 = Interrupt has occurred. Write 1 to clear the interrupt.
...    | ...      | ...    | ...
1      | foo_en   | RW     | 0 = Foo is disabled. 1 = Foo is enabled.
0      | irq_en   | RW     | 0 = Interrupts are disabled. 1 = Interrupts are enabled.

Suppose a programmer wants to implement "Change this register to set foo_en to 1." They write this in the obvious way as reg.modify(|_, w| w.foo_en().set_bit()). svd2rust does the simple and 'obvious' thing: it reads the register, computes val | 1 << 1, and writes that value to the register.

However, this naïve API and implementation has led to the wrong result. If irq_stat contained 1 when it was read, we would write back a 1, which would clear that field! This is not what the programmer wanted!

To work with such a register, the programmer needs to ensure that every time they modify() that register, they handle the W1C register correctly. To implement "Change this register to set foo_en to 1," they actually need to do reg.modify(|_, w| w.foo_en().set_bit().irq_stat().clear_bit()). This is counterintuitive, since their intention was to not change the value of int_stat. Even the name clear_bit() is misleading, because the effect on the register will be to not clear that field. When the programmer wants to "clear" the field, they must use the set_bit() function.

(Note: all of this applies to W0S ("write-0-to-set") fields as well. When the programmer wants to "set" the field, they must use the clear_bit() function, etc.)

This is backwards, confusing, and dangerous. I'll grant that one could argue that svd2rust is simply implementing the obvious actions, and that the onus is on the programmer to know what the fields in the register are and how to use them correctly. However, given how svd2rust provides so much other protection such as not accessing reserved bits, or only writing allowed enumerated values, I'd argue that this naïve handling of W1C fields is dangerous. The behavior if the programmer makes a careless omission is exactly what they wouldn't want, and the only remedy is to check all of their register accesses by hand to ensure that they handled all W1C and W0S fields correctly.

I suggest that svd2rust should provide different handling for W1C and W0S fields. Some ideas on how this could be done:

• Offer alternatives to clear_bit() and set_bit() on W1C and W0S fields. Provide functions with distinct names whose purpose is clear, such as change() (write 1 to a W1C, and 0 to a W0S), no_change() (write 0 to a W1C, and 1 to a W0S), and change_if_set() (write the same value that was read).
• Abolish the clear_bit() and set_bit() functions on W1C and W0S fields, on the basis that their names are inherently confusing and describe the exact opposite of what writing that value will accomplish.
• Offer an alternative to modify() that does the equivalent of w.field().no_change() on all W1C and W0S fields before calling the closure function.
• Abolish modify() on any registers that have W1C or W0S fields (or maybe only ones that have a mix of RW and W1C/W0S fields) in favor of the alternative, on the basis that modify() is always dangerous on such registers since a careless omission by the programmer can lead to unexpected and undesired behavior.
• Create a new API for such registers that forces programmers to explicitly specify what to do with all W1C and W0S fields. In effect, this would create a form of tri-state logic, such that when w is initialized, RW fields have the value that was read, but W1C and W0S fields are marked as "unknown" or "unhandled". All such fields must be deliberately set to some value by the closure; not doing so would be an error (hopefully a compile-time error).

(Obviously I don't expect all of those ideas to be accepted, particularly as it may break backwards compatibility. Just consider them as brainstorming how this could be resolved.)

Thanks for the writeup! I think this is a good point and worth investigating, though I'm not sure how easy any wins will be.

• In a lot of stm32-rs SVD patches, we add an enumerated value called clear with a value of 1, so for those fields you can call reg.write(|w| w.field().clear()) for example. In practice this also gives a reasonable API, but the clear_bit() method is still there, and of course w.field().clear() and w.field().clear_bit() would do the opposite thing.
• The SVD spec does have an attribute for fields to indicate this property, the modifiedWriteValues attribute which can take values like oneToClear, but I've not seen it in the wild (at least in ST's STM32 SVDs), so I don't know if it's very prevalent in SVD files, and therefore even if svd2rust did handle it specially, many fields would still have the wrong methods. They could eventually be fixed on a per-SVD basis, at least.

https://github.com/rust-embedded/svd2rust/commit/189548a9b29ccfdc7890f613d48ac4471175e635

* _but_ the `clear_bit()` method is still there

We could use deprecation mechanism to make people use methods that are generated from enumeratedValues instead of set_bit/clear_bit where it is possible. Not only for this case but for any. We can add --deprecate features which when enabled will generate #[deprecated] on set_bit/clear_bit when variant is present.

@adamgreig Thanks, I'm glad my explanation makes sense and that I could sell you on the idea. :-)

Unfortunately, you're right, the only benefit would be for SVD files that use the modifiedWriteValues attribute. However, the same is true for any protection that svd2rust provides. E.g., multi-bit fields that don't use enumeratedValues provide no protection, and must be wrapped in unsafe since the onus is on the programmer to ensure that only safe, allowable values are written there. We can only hope that people will write SVD files that are as descriptive as possible, and perhaps having handling for these more uncommon situations would encourage them to do so.

You may have guessed, but this was inspired by a real life problem. In my day job, I'm working on a proprietary ASIC and ran into a situation just like the example I showed. In that instance, the SVD does use modifiedWriteValues with oneToClear, so we could have avoided the problem if svd2rust had provided better APIs/protection for W1C fields. Luckily, we caught the problem during testing, but this is what made me realize the only defense is programmer vigilance, checking whether any register with a W1C/W0S field is accessed in the program and checking that all of those accesses handle the W1C/W0S fields appropriately. :-(

The commit @burrbull pointed out (https://github.com/rust-embedded/svd2rust/commit/189548a9b29ccfdc7890f613d48ac4471175e635) actually adds support for this to svd2rust (it will generate clear_bit_by_one() and set_bit_by_zero() methods where appropriate, as well as toggle_bit()), It also removes set_bit() and clear_bit() from those bits, so I think it should address your idea? It's not yet merged but will probably be in the next svd2rust release (sorry, I realised on edit that it's not merged yet).

We could use deprecation mechanism to make people use methods that are generated from enumeratedValues instead of set_bit/clear_bit where it is possible.

I often still use set_bit()/clear_bit() even when enumeratedValues are present as sometimes it feels clearer (it's always clear what value it writes, anyway...). Not sure about marking them deprecated when enumeratedValues are available.

The commit @burrbull pointed out (189548a) actually adds support for this to svd2rust

Not in master yet: #541

It's certainly an improvement :-D but IMO it doesn't address the biggest problem: if you do reg.modify() and don't do anything to the W1C/W0S fields, the register's value will change, because all W1C bits that were 1 will get cleared, and all W0S bits that were 0 will get set. Sometimes that's what you want (e.g. an IRQ register whose fields are all W1C), but in fields that have a mix of RW and W1C/W0S fields, it's far too easy for the programmer to write code thinking only about a RW field they want to change, overlook the W1C/W0S fields, and produce code that doesn't do what they intended. svd2rust doesn't do anything to help them detect an omission like that, and IMO this is a flaw in its API.

I think it would be much safer if there were some mechanism, any mechanism, that could help detect or prevent such errors.

Should be fixed by #673 Reopen if it is not.