Should RW1C bit fields have `set_xxx` and `clear_xxx` which do actually *set* and *clear* bits?

[toku-sa-n]

Currently, there are two methods for the RW1C bit: clear_xxx and set_0_xxx. The former sets the bit to 1, the latter sets it to 0.

Originally, only the former existed, because I thought there was no need to write 0 in the RW1C bit. However, as pointed out in #146, there are actually situations where it is necessary to write 0 to prevent the bit from being erased, which is why the latter was added in #148.

I didn't think anything when I merged #148, but later I thought this was obviously confusing. For example, the same clear_xxx method exists for RW bits, but this method sets the corresponding bit to 0.

Should I fix this and have the RW1C bits also have set_xxx/clear_xxx methods that actually set the bits to 1/0?

One problem is that the clear_xxx method will have the exact opposite meaning in a later version of this change, and thus must be clarified in the changelog.

[paulsohn]

To prevent that said confusion, method docs should specify whether each method actually assigns 0 or 1.

Sets the xxx bit to 1. (on RW/RW1S bits, set_xxx) Clears the xxx bit by assigning 0 to the bit. (on RW bits, clear_xxx)

Clears the xxx bit by assigning 1 to the bit. (on RW1C bits, clear_xxx) Prevents the xxx bit from being cleared on write, by assigning 0 to the bit. (on RW1C bits, set_0_xxx) ( + for both methods, perhaps adding This bit is 'write 1 to clear' bit. can be more informative.)

Of course, we can rename either clear_xxx method to something like erase_xxx, but the docs update should be preceded, since without enough documentation the actual behavior of the method is opaque for crate users.

Off-topic: suggestions about renaming (which may lead to a breaking change)

1. Provided that each method informs its exact behavior in its doc, I suggest that set_0_xxx methods should be renamed. How about preserve_xxx or protect_xxx?

2. Why don't we have a setter method for both 0/1 bits, on RW bits? The proposed assign_xxx method would be equivalent to:

fn assign_xxx(&mut self, bit: bool) -> &mut Self {
    if bit { self.set_xxx() }
    else { self.clear_xxx() }
}

This, however, is an inconsistent naming convention since field setters are set_xxx but this bit setter can only have name assign_xxx. It's because of the English ambiguity that 'setting a bit' possibly means both 'raising the bit to 1' and 'assigning 0/1 value to the bit'.

And here's my second thought: within this crate, fix the verb 'raise' for the write-1-to-set-1 action, and use 'set' to only indicate the assignment. This means our previous set_xxx should be renamed into raise_xxx, and the proposed assign_xxx takes the name set_xxx and accepts a boolean argument.

To sum up, our final bit setter methods and their documentations would be something like:

(RW bits)

• set_xxx(bool) : Assigns the given value to the xxx bit.
• raise_xxx() : Raises the xxx bit to 1.
• clear_xxx() : Clears the xxx bit to 0.

(RW1S bits)

• raise_xxx_if(bool) : Conditionally raises the xxx bit to 1. If the argument is false, the bit will be left unchanged. (I added this only for completeness' sake. Not quite necessary.)
• raise_xxx() : Raises the xxx bit to 1.

(RW1C bits)

• erase_xxx() (or clear_xxx()) : Clears the xxx bit by assigning 1 to the bit. (I would choose 'erase' because I feel the verb 'erase' more deliberate, and 'clear' has more nuance of write-0 action.)
• preserve_xxx() : Prevents the xxx bit from being cleared on write, by assigning 0 to the bit.

---

Sorry for my delaying response (on this, and another PR I'd created) but I feel stuck on my personal project. I'll work a PR for this once my project has been settled.

[toku-sa-n]

Thanks for the suggestion, and sorry for the delay.

To prevent that said confusion, method docs should specify whether each method actually assigns 0 or 1.

Certainly, it's good to mention whether a method sets 0 or 1. It's so great that it doesn't need a breaking change, although I'm not sure how many users will read the docs to check the actual behavior of setters and clearers.

Provided that each method informs its exact behavior in its doc, I suggest that set_0_xxx methods should be renamed. How about preserve_xxx or protect_xxx?

I understand the purpose of renaming set_0_xxx to preserve_xxx or protect_xxx, but I'm not sure how clear these names are.

Why don't we have a setter method for both 0/1 bits, on RW bits?

Initially, this crate provided set_xxx(bool), but later I split it into set_xxx and clear_xxx which set 1 and 0, respectively because I thought having a boolean parameter was a code smell (which is generally true, but I'm not sure it applies to this case.)

And here's my second thought: within this crate, fix the verb 'raise' for the write-1 action, and use 'set' to only indicate the assignment. This means our previous set_xxx should be renamed into raise_xxx, and the proposed assign_xxx takes the name set_xxx and accepts a boolean argument.

"raise" is somewhat a good choice. Although I haven't seen a case where the word was used to mean setting 1, it can easily be associated with the operation. I think "set" and "assign" are better, however, as many people use "set" to assign 1 (Google search result) and "assign" means exactly what the method does.

To sum up, our final bit setter methods and their documentations would be something like:

I'm not sure whether letting different types of bits have different names for the same operations is good. On the one hand, users can notice the misrecognition of bit types by compile errors. On the other hand, such method names are somewhat confusing, and it might be better to let all bits have the same names and same operations (e.g., set, clear, assign for assigning 1, 0, the passed value, respectively), and write docs what and which bits are RW/RO/RW1C/RW1S and a note about handling RW1C.

---

To sum up, we can clearly whether a method sets 1 or 0 by updating the docs without introducing a breaking change. For renaming methods, using "set", "clear" (, and "assign" if necessary) are better.

---

Sorry for my delaying response (on this, and another PR I'd created) but I feel stuck on my personal project. I'll work a PR for this once my project has been settled.

No problem. I'm too delayed.

[paulsohn]

Certainly, it's good to mention whether a method sets 0 or 1. It's so great that it doesn't need a breaking change, although I'm not sure how many users will read the docs to check the actual behavior of setters and clearers.

Currently clear_xxx for RW1C bits are (at least for me) undoubtedly ambiguous. That's because each register type represents, technically speaking, not the MMIO register itself but some bitmap type which has same layout to the register except that the write behavior is 'write-1-to-set' and 'write-0-to-clear'.

For example, I wanted to clear PORTSC CSC bit of ith port, which is RW1C, so I expected that I should write something like this:

registers.port_register_set.structural_at_mut(i).portsc.update_volatile(|portsc| {
  portsc.assign_connect_status_change(true);
});

(allow me to use structural indexing syntax here for convenience.)

And for the alternatives of hypothetical (but good-to-have) .assign_connect_status_change(true), I searched for a single method which assigns 1 to the bit. But all I got were .set_0_connect_status_change() and .clear_connect_status_change(), which was especially confusing because 'clearing' means 'setting 0' on the 'PORTSC bitmap' level, which I explicitly manipulate in the closure, (despite that 'clearing' actually means 'writing 1' on the actual PORTSC register level) so I misunderstood that there were two identical methods with different names, and no explicit 'writing 1' method. After expanding all source codes and macros, I convinced myself that .clear() describes the behavior of register level and this was what I wanted.

Personally I don't want others spend their time only for confirming what a method actually does. If the behaviors were well-documented, at least modern-IDE users just like me will immediately read them when a method is selected from the autocomplete menu, and that's enough. Besides, docs are there to be read. Having clearer docs is not a breaking change, neither something you should doubt about.

[toku-sa-n]

Certainly, you're correct, and we should describe whether a method sets 0 or 1. Are you going to write them, or do I do?

For the methods, after my second thought, it may be better to define only the assign or set method which takes a boolean value representing a bit value (true and false corresponding to 1 and 0, respectively) given that the word clear is ambiguous here (and that you're not the only one. Another case is https://github.com/rust-osdev/xhci/pull/128, at this point, I should've thought of renaming clear.)

[paulsohn]

Made it. https://github.com/rust-osdev/xhci/pull/160

RW1C docs are now

clear_xxx : "Assigns 1 to the xxx bit. On register write, this results in clearing the bit." set_0_xxx : "Assigns 0 to the xxx bit, preventing the bit from being cleared on write."