Generate bool accessors for single-bit fields

[whitequark]

This makes using such fields significantly more ergonomic, as
previously they would have been u8s, leading to unnecessary
conversion and silent truncation in downstream code.

The bool writers are never unsafe because it is meaningless
to have a writable field that only admits one (or worse, zero)
valid values.

[whitequark]

Oh, and as a minor but sweet side effect, this change makes documentation much easier to read.

[japaric]

Thanks for the PR, @whitequark.

I don't think I'm going to accept this PR as it is since it seems to throw away enumeratedValues information if the field has size 1 bit. This results in a loss of readability. Something like this:

// nrf51.svd
timer0.mode.write(|w| w.mode().counter()); // or mode().timer()

would become

timer0.mode.write(|w| w.mode(false));  // false mode?

if I'm reading this correctly.

I'd be more willing if the enumeratedValues information was preserved.

Also note that the code generation is designed such that adding enumeratedValues information to a SVD file and then running it again through svd2rust expands the previous API in a backward compatible way and I'd like to preserve that property.

[homunkulus]

☔ The latest upstream changes (presumably c758eb7) made this pull request unmergeable. Please resolve the merge conflicts.

[whitequark]

I'd be more willing if the enumeratedValues information was preserved.

I have completely missed this case, thanks.

Also note that the code generation is designed such that adding enumeratedValues information to a SVD file and then running it again through svd2rust expands the previous API in a backward compatible way and I'd like to preserve that property.

I have not previously realized that this is a goal. I agree that it seems clearly desirable, and while it would not be a deal breaker for me personally, I can certainly understand if it is for you. As such, let's change the goals a bit.

For this PR, I will, much more modestly, change the bits() -> u8 accessor to bit() -> bool. This would fix a large part of the ergonomics issue that prompted me to write this PR.

After that, I suggest rethinking the approach to nesting in the generated code a bit. The existing code generator makes the documentation very hard to use, particularly when many of your fields are single-bit, many of our other fields have no reserved bit patterns, and many of your registers only have one field (perhaps because all of the registers require 32-bit access even if the peripheral doesn't). In this case, writing code against the generated API becomes an endless maze of indirection that serves no purpose.

It is, indeed, nontrivial to rework the code so that later you can add more detail (fields and enumerated values). As a compromise between backwards compatibility and ergonomics, I suggest pulling the bits function out of the proxies, and not generating the proxies at all unless there's anything nested inside. So, if you don't have any <enumeratedValues>, you don't have the field proxies defined at all, and access the bits using periph.reg.read(|r| r.field_bits()) and periph.reg.write(|w| w.field_bits(0xa)); if you don't have any <fields> then you don't have the register proxy defined at all and access the bits using periph.read_reg() and periph.write_reg(), and if you have both <fields> and <enumeratedValues> then nothing changes in the API from the current situation at all.

I hope that this will cut down the majority of noise in the documentation, and make it much more convenient for casual use.

[whitequark]

@japaric I have now updated this PR. An alternative approach to changing the name depending on the width would have been to call the accessor raw(), but I do not like it because it implies that the accessed value is somehow not fully representative of the underlying semantics, whereas talking about "the GPIOA enable bit" is very common.

[istankovic]

We definitely need something like this in order to improve ergonomics of the common case.

[japaric]

Thanks for the changes @whitequark. This now looks good me. Could you update the crate level documentation (src/lib.rs - that's what shows up in docs.rs) to reflect the changes?

re: documentation being hard to navigate. I agree. When I designed the API I was assuming autocompletion would always be used to write code. So in my mind using the API should have looked like this:

peripheral.register.write(|w| w._)
                                |fieldA - enables / disables the timer
                                |fieldB - oneshot or contiguous
                                |fieldC - enables / disables interrupts
                                |(..)

// then
w.fieldA._
         | enabled() - enables the timer
         | disabled() - disables the timer

In practice, racer doesn't work that well when closures or type inference is involved.

[whitequark]

@japaric done

[japaric]

@whitequark after looking at this again (after properly sleeping), I'm not fully convinced about all the changes here.

• the bits -> bit change makes sense to me. It better reflects that field is
  1-bit in size.

• the u8 -> bool change feels like it reduces readability. From the documentation changes:

-//! i2c1.cr2.write(|w| unsafe { w.sadd0().bits(1).sadd1().bits(0b0011110) });
+//! i2c1.cr2.write(|w| unsafe { w.sadd0().bit(true).sadd1().bits(0b0011110) });

bit(1) would have been clearer, IMO: "Set this bit to 1". bit(true) doesn't immediately tells me that the bit is being set high. I have no way to tell from just the invocation if true actually maps to 0 under the hood. bit(1) also requires less typing.

I do agree that bool is better than u8 when it comes to constraining the input.

I think this would be even clearer if it used methods: sadd0().set() and sadd0().clear() (or set_low/set_high). Same with if i2c1.c2r.read().sadd0().is_high() (or is_set). This also deals with restricting the input.

• the 'the bit method is never unsafe' change seems to have been reverted. Was that a change of heart? I thought that was one of the main motivations of this change (to reduce the unsafe noise).

Would love to hear others' opinions

[whitequark]

I think this would be even clearer if it used methods: sadd0().set() and sadd0().clear() (or set_low/set_high). Same with if i2c1.c2r.read().sadd0().is_high() (or is_set). This also deals with restricting the input.

"Low" and "high" aren't applicable to bits, only signals, and in fact this would add ambiguity because a logical one could be represented by either low or high signal level. set and clear are fine, I've been thinking about these just today. I have no strong opinion on is_set other than it feels extraneous (it's purely an alias of bit() and I might never use it.

Regarding bit taking u8, I am wholly unconvinced. Currently, you choose the smallest width integer type that can fit the field--and I think this makes sense, fields with a size that's an integral number of octets are very common, and modeling them properly (i.e. such that with every legal input, no bits are lost during a write) is important. What my change does is merely extends this to work properly on fields of width 1.

The change of bit to take bool in particular is motivated by functions that don't merely set a bit to a known value but rather take an argument. Adding a as u8 in every such case is a major code smell, in my opinion. The inverse case is also quite troublesome: you could easily accidentally compare it with an u8 variable whose valid range is wider than [0;1], or you would have to manually narrow it when performing a match, etc. I think this is a clear improvement.

the 'the bit method is never unsafe' change seems to have been reverted. Was that a change of heart? I thought that was one of the main motivations of this change (to reduce the unsafe noise).

Somewhere in the process I've added the equivalent of this change to my SVD file generator and, it seems, then never noticed removing it when refactoring the svd2rust patch. I of course still think it's a major improvement in ergonomics and will add it back.

[istankovic]

set and clear seem nice, +1 on that one.
Regarding the u8 vs bool, I think @whitequark has good reasoning there (I do feel the "less typing" argument, but with set and clear I think it's a non-issue).
Regarding safety, I think the method should definitely be safe.

[whitequark]

Updated

[japaric]

Thanks for updating the PR @whitequark. I stil don't think that we should (over)use bool to represent bits. I think it would make more sense to use a enum Bit { Zero, One }. It has better readability, IMO:

// READ
// this PR: if the bit is ... what?
if peripheral.register.read().bitfield().bit() { .. }

// better: if the bit is one
if peripheral.register.read().bitfield().bit().is_one() { .. }
// or
if peripheral.register.read().bitfield().bit() == Bit::One { .. }

// with enumerated values (bitfield = mode)
if peripheral.register.read().mode().bit().is_one() { .. }
if peripheral.register.read().mode().bit() == Bit::One { .. }
if peripheral.register.read().mode().is_output() { .. }
// or
if peripheral.register.read().mode() == Mode::Output { .. }

// WRITE
// no enumeratedValues
peripheral.register.write(|w| w.bitfiled().set()); // or clear()
peripheral.register.write(|w| w.bitfiled().bit(Bit::One));

// with enumeratedValues
peripheral.register.write(|w| w.mode().set()); // or clear()
peripheral.register.write(|w| w.mode().bit(Bit::One));
peripheral.register.write(|w| w.mode().output()); // or input()
peripheral.register.write(|w| w.mode().variant(Mode::Output));

[whitequark]

I stil don't think that we should (over)use bool to represent bits. I think it would make more sense to use a enum Bit { Zero, One }. It has better readability, IMO:

I think almost every example above is non-representative as they are missing the key motivation for having the .bit(x) accessor as opposed to .set()/.clear() accessors: to support code that receives the value from somewhere else. Consider something like the following:

#[derive(Ord)]
enum Mode { Active, LowPower, Sleep };

fn enable_clocks_for(mode: Mode) {
  clocks.uartclk.r0().bit(mode <= Active);
  clocks.uartclk.r1().bit(mode <= LowPower);
  //etc
}

My point is that the fact that it is a bit is an implementation detail, logically, it is a toggle. There's really no reason to newtype bool and then add useless conversion everywhere. Can you show any example where your bool newtype would prevent a bug from being added?

That said I think adding .is_set() and .is_clear() is a definite improvement in readability and I want it as well.

[cwoodall]

I would really discourage: Bit { Zero, One } over true, false. It does not give any more information and may in fact be misleading/harder to work with. I can't apply logical operators to Bit { Zero, One } without additional fiddling for example (for toggling, etc).

I agree that in the case of .bit() being able to feed boolean values has a lot of ergonomics in the generic case, and in the case where more information is available from an enumeratedValue type convention should dictate that the more descriptive type is used.

[japaric]

Consider something like the following:

IMO, that code looks like it has been optimized for writing rather than for reading. I would use match on the enum and set/clear personally.

OK. It seems fine to just use bool since you can also use {,is_}{clear,set} if you want to. I'll review this as it is. @whitequark you can send is_set / is_clear as a follow up PR.

[whitequark]

IMO, that code looks like it has been optimized for writing rather than for reading. I would use match on the enum and set/clear personally.

Perhaps the choice of the example was not the best. But nevertheless I disagree on the underlying point, having to insert a match/conditional every time a bit could be either set or cleared indicating an on/off state would be completely unnecessary boilerplate. svd2rust is supposed to reduce that not add it.

[japaric]

The bit method on W could be simpler, I suppose, but it works OK.

    pub fn bit(self, value: bool) -> &'a mut W {
        const MASK: bool = true;
        const OFFSET: u8 = 19;
        self.w.bits &= !((MASK as u32) << OFFSET);
        self.w.bits |= ((value & MASK) as u32) << OFFSET;
        self.w
    }

@homunkulus r+

[homunkulus]

📌 Commit 3405452 has been approved by japaric

[homunkulus]

⌛ Testing commit 3405452 with merge 0a1d8f6...

[homunkulus]

☀️ Test successful - status-appveyor, status-travis
Approved by: japaric
Pushing 0a1d8f6 to master...