Drop return value from register accessor closure type (big breaking change)

[couchand]

The methods Reg::write, Reg::write_with_zero, and Reg::modify all expect a closure that accepts a mutable reference and then must return the same mutable reference. This would seem to violate the usual API design guidance.

My reading of the code & examples is that the rationale is to make one-liners easy, for instance, the example in the docs on Reg::write https://github.com/rust-embedded/svd2rust/blob/29129c0e4e261bf4c38057f7585e7ddfc05fafd0/src/generate/generic.rs#L95-L99

However, the ramification of this design on real-world code is not as nice. See, for instance, in stm32-rs or avr-hal.

The last statement in each register update must omit the trailing semicolon, making it inconsistent with the rest of the statements in the closure, causing issues with, for instance, refactoring of the method.

If the return value were removed, the real-world examples would see a maintainability improvement, and the example on Reg::write would hardly need to change at all, either:

 periph.reg.write(|w| {
     w.field1().bits(newfield1bits) 
         .field2().set_bit() 
         .field3().variant(VARIANT);
 });

or

 periph.reg.write(|w| {
     w.field1().bits(newfield1bits);
     w.field2().set_bit();
     w.field3().variant(VARIANT);
 });

My preference is for the latter, and as an aside, also dropping the return value of the field modifiers (though since that's simple enough to ignore I'm not as concerned about it).

---

There's also a significant improvement in terms of the user's mental model (which goes to the more general API design guidelines). As it stands, there's potential for confusion about how exactly these mutators do their job:

• they take a mutable reference, so you'd think that mutating that is what matters
• but they require you to return a mutable reference, so is that what matters?

The unfortunately reality is that it's the latter, and the input parameter just happens to be the way you get the relevant struct. This seems quite awkward.

The potential for misuse of the current API is relatively low, since it's not simple to conjure a reference. But that can't be enforced by the compiler in the way that the same API without the return value does let the compiler enforce correct usage. And since the generated code is frequently integrated with hand-written code in a register access crate, pub(crate) is hardly any protection.

---

The biggest concern that I can see with this proposal is a purely practical one: how would it actually be implemented? It would amount to a big breaking change across all users of this crate. That sounds scary (but not impossible!).

I'm not sure what the policy of this crate is regarding breaking changes in the generated code. Certainly it would need to be approached with care and great tact, and lots of warnings/advice/autoconvert-tooling/something would need to be provided. But the API without the return value seems so much stronger to me that I thought it was at least worth bringing up.

---

I originally suggested this in a bit of a throwaway comment in #463, but since running into it again with #475/#477, I thought I'd log it as a separate issue so there's at least a place to discuss it.

If breaking changes were on the table, I'd suggest changing the API of Reg::write. Requiring the closure to return the input doesn't seem like the right thing to do. It sows doubt about how exactly the changes propagate, and it makes one-liners slightly shorter at the expense of making multi-line closures an extra line.

[hannobraun]

As a PAC and HAL author, I fully agree with this. The current API is extremely confusing, and I'd rather weather a breaking change like this than carry this wart forward into eternity.

[therealprof]

Nominating this for a broader discussion in the next meeting.

[cr1901]

This would seem to violate the usual API design guidance.

What guidelines in the checklist are being violated?

Anyways, needing to remove the semicolon from the last statement doesn't bother me that much. I'm not sure why the write methods receive and return a proxy compared to directly doing the writes. "It makes one-liners slightly shorter at the expense of making multi-line closures an extra line" could be one reason. But I seem to recall another reason is that the proxy type is used to combine disjoint reg field writes into one write. How would that be implemented?

• they take a mutable reference, so you'd think that mutating that is what matters
• but they require you to return a mutable reference, so is that what matters?

If this PR isn't possible to implement, I think a doc change making clear that the w sent into write/modify is a proxy type meant to be returned, as well as the rationale as to "why are things done this way" would be reasonable for alleviating confusion.

[adamgreig]

I mostly use PACs directly rather than writing HALs, but in my experience almost all uses of write and modify look like reg.modify(|_, w| w.field().variant());, where it's very natural to omit the closure's semicolon, and this change would basically mean going through the code for every register access to add a semicolon. It seems like for this simple use-case there's no advantage to the change but it requires some extra syntax. Maybe in the HAL use-case it's more common to have larger blocks of code inside the closure, where this change does improve ergonomics?

In other words, I'm not sure whether there's any advantage for my typical use case which would outweigh the (fairly minor but extensive) breaking change incurred.

In general in Rust it seems pretty common to just have that omitted trailing semicolon on the last line. If larger closures wanted to remove the inconsistency they could always use return w; at the end, right?

[teskje]

Maybe in the HAL use-case it's more common to have larger blocks of code inside the closure, where this change does improve ergonomics?

In the HALs you usually find these larger blocks during initialization of peripheral abstractions where all fields in a register are set to their initial known states. I think not having to omit that final semicolon makes that a bit nicer, but I don't mind too much personally. As you say, in Rust one is used to omit the last semicolon anyway.

What I would like to go away is this common pattern for writing these larger blocks of register manipulations:

reg.write(|w| w
    .field1().variant1()
    .field2().variant2());

This doesn't play nicely with rustfmt, which re-layouts this to something like:

reg.write(|w| w
    .field1()
    .variant1()
    .field2()
    .variant2());

... which is not readable at all IMO. That can be easily solved by putting each field access into a separate statement instead of chaining, but it seems many don't think about that. We could enforce that better by, as @couchand mentioned above, not returning the write proxy from the field modifier methods.

I think that second breaking change would help quite a lot with making HAL code more readable. I realize this is not what this issue is about, but maybe if we would bundle both breaking changes together it would be worth the effort?

[David-OConnor]

From an API perspective, I've been thinking about the feasibility of a wrapper like this. What are we trying to do? We're either replacing the value of a whole register, (write) replacing the value of a field (modify), or reading one of these. We just need to specify the action, the register, optionally the field, and if writing, the value. The closure syntax feels unecessary, and the modify vice write isn't self-documenting.

The specific methods like enabled(), is_clear(), is_set(), bit() etc seem like a nice way to be explicit, but I find they increase cognitive overhead. All we're doing is writing integers (or booleans) to registers and fields; I'm not certain the wrapping methods are worth the complication.

write(reg, value);
write_field(reg.field, value); 
read_field(reg.field);

// Or:
reg.write(value);
reg.field.write(value);
reg.field.read();

// Instead of the current
reg.write(|w| w.bits(value));
reg.modify(|_, w| w.field.bits(value);  // or ...enabled(), or ...set_bit() etc
reg.read().bits();

[bradleyharden]

@David-OConnor, I'm very new to both Rust and the embedded Rust ecosystem but this was my understanding. Someone please correct me if I'm wrong.

It seems like what you're asking for already exists. The bits function on every (multibit) field lets you set the value. But it is marked as unsafe precisely because it can't guarantee that the user-supplied value is valid without runtime checks. The individually named functions choose valid values from an enum listing all possible values. Right now, you could do what you ask safely using the variant functions on each field, but then you would need to import the enum associated with each field, which is a pain.

I assumed the closure is there to ensure that modifying multiple fields of a single register is compressed to as few instructions as possible. I think it might be harder to do that with separate statements, since each register is a VolatileCell.

[David-OConnor]

From what I understand, why you can currently do is the 3rd example I posted. I'm advocating for a simpler syntax. Sure, you can save some code if you're doing multiple edits. Anecdotally, this is a minority of cases, and the closures are boilerplate.

[therealprof]

We tried to discuss this in the meeting today but couldn't due to lack of discussion participants. Can anyone (mabye @couchand) summarized the current state of discussion and the questions which still need addressing?

[ryankurte]

it's somewhat tangential / not something to get stuck on here, but, one of the other things i think is worth consideration when looking at this is, whether alternate approaches either result in or can improve the generated docs for registers / bitfields / flags. some excellent improvements have been made recently but, seems to me there's still some room to smooth over the API?

as an example (just the most recent thing i used), the SAMD21 ADC requires you to first find the ADC struct 1, then the register block 2, then 50/50 you click the register you want, which tells you you can modify it 3 or the type that shows you what you can actually set 4. We can probably mitigate this this with human-generated doc comments but, it'd be neat to find some what to tie all these components together in a more streamlined way imo.

[couchand]

For @therealprof and the rest of the team, here is my attempt at summarizing the main decision points. I will follow with my own personal opinion in a later comment.

There seem to be broadly three threads here:

1. can we, should we, and how would we make breaking changes to generated code?
2. is the register mutation design sufficiently imperfect to justify breaking changes?
3. what shape should the target API for register mutation take?

---

Breaking changes in the generated code

Problem statement: There's not a clear versioning story for this tool.

• People want stability, but sometimes things need to change. Semver provides useful guidelines for libraries, but not as much for tools, let alone tools that generate code that is directly incorporated into a library.
• Crates that depend on svd2rust generally don't declare a dependency on any particular version.
• There's not an explicit support/change policy for this project.

Presumably this project would like to support dependent projects' goal of following semver. Semver implies that any breaking changes in the generated code would translate to a new major version in every dependent project. This concern should not be taken lightly.

---

Concerns with the current API

Problem statement: Are the warts in the current API big enough to justify all the trouble?

A number of commenters have suggested that the current API is fine, or that it is good enough to not warrant the trouble of breaking changes. Others have raised additional points beyond what was written in the original post.

The concerns with the current API include:

• The closure type can be confusing. It's not clear why it takes and returns the same argument, both from a user's perspective as well as from an implementor's perspective (as indicated by the uncertainty here).
• Refactoring of large register initialization blocks is mildly impeded by the mismatch in the last line.
• The usage patterns encouraged by the current API don't play nicely with rustfmt.
• Closure syntax for register modification seems awfully cumbersome for what should be a simple operation.

---

Ideal design for the API

Problem statement: If changes are deemed to be warranted, what is the target design that the API should be moving to?

My original proposal was for a minimal change. Others have suggested that, as long as breaking changes are being made, it's a good opportunity to make more significant improvements. Some key design questions:

• Can we get rid of the closures entirely?
• If we did, could we still support combined updates to multiple fields in a single register?
• What's the right level of abstraction for the generated API? After all, we all know we're just moving bits around under the hood.

There are a few important considerations to take into account:

• The performance of the generated code is paramount. We're producing a foundational abstraction for devices that are in many cases extremely limited. Size of the compiled artifact and runtime performance are critical. Compilation time is a secondary, but still important, concern.
• Current code samples that use the single-line syntax (e.g. reg.modify(|_, w| w.field().variant());) are extremely common, so we should do what we can to not break them.
• The generated documentation has been getting better, but there's still lots of room for improvement.

[couchand]

This comment contains my personal opinions on the above discussion points. Feel free to take them or leave them as you like, I'm just one crank on the Internet.

Breaking changes in the generated code

It's a non-starter for the answer here to be, "breaking changes are never accepted". It's reasonable for the bar those changes need to clear to be quite high, since there's such a wide ecosystem impact. But there absolutely must be a clear, well-defined path for those changes, when they're deemed necessary.

There are a few things we can start to do today to help. The biggest one is to document a clear policy and process, with some actionable steps downstream crates can do to help future-proof our operations.

Strawman proposal:

1. If breaking changes in the generated code are warranted, new versions of this tool must be able to generate both the old version and the new version for some period of time.
2. Dependent crates must specify a dependency on a specific version of svd2rust.
3. Dependent crates should be discouraged from releasing 1.0 until this crate reaches 1.0.
4. New versions of svd2rust should be published to crates.io frequently, so that downstream crates aren't tempted to rely on the bleeding edge.

---

Concerns with the current API

The current API is not good enough. We can do better to make it approachable, efficient, and clean. This can make a major impact on the usability of the whole Rust embedded ecosystem.

---

Ideal design for the API

I approach this from two sides:

1. What is the minimal set of changes to make the API clean and obvious?
2. What would the API look like if we were designing it from scratch today?

Version 1: minimal changes

I think the answer to (1) is just both of my original suggestions: change the closure type to remove the return value, and stop returning the register write proxy from the field modifiers. This addresses most of the concerns raised above:

• The API becomes tighter, so there's no question about how changes propagate. This signature is simple and obvious:

fn write<F>(&self, f: F) where F: FnOnce(&mut W);

• Large initialization blocks become more uniform & play nicely with rustfmt.

periph.reg.write(|w| {
    w.field1().bits(newfield1bits);
    w.field2().set_bit();
    w.field3().variant(VARIANT);
});

• One-liners need no change. Since the field accessors return () and the write closure expects (), the one-liners work as-is.

reg.modify(|_, w| w.field().variant());

Version 2: total redesign

On the other hand, if I were designing this API from scratch per (2) I might do things differently.

• As others have mentioned, it's perhaps less common to accumulate multiple field changes to apply at once than it is to just tweak a single field. On that assumption, we should design the API to favor the more common case.
• I like @David-OConnor's suggestion of stepping through fields directly rather than with an accessor function. I'm not sure if it's feasible, but it would be very nice so I'm using that in these code fragments.

periph.reg.field1.set_variant2();

// SAFETY: The value 0x42 is always valid for this field.
unsafe {
    periph.reg.field2.set_bits(0x42);
}

periph.reg.flag_field.clear();

I'd really like registers that don't have subfields to have a safe, simple, register-sized write, if possible:

*periph.plain_byte_register = 0x42;

// or, if needed
periph.plain_byte_register.write(0x42);

For that matter, I'd rather not have to step through a call to .read() to get the current values, either:

if (periph.reg.field1.is_variant3()) {
    // ..
}

let local_value = periph.reg.field1.bits();

We still want to support the use case of accumulating multiple fields in a single register. I think this is stronger when made explicit by accumulating the fields first and then issuing the write operation.

Sure, you can always use the escape hatch of the unsafe set_bits:

// SAFETY: I promise I know what I'm doing.
unsafe {
  periph.reg.set_bits(0x42);
}

But we can make it better by providing a safe wrapper. Something like:

let mut new_value = periph.reg.new_hypothetical_accumulator_type();
new_value.field1.set_variant2();
new_value.field2.set_variant5();
periph.reg.write(new_value);

To prevent misuse, we probably want this HypotheticalAccumulatorType to be marked #[must_use] and then the register's write() method would take it by value. We could also make it Clone so it could be used that way as long as you're explicit about it:

const MY_DEFAULT_VALUE: periph::reg::HypotheticalAccumulatorType = make_my_default();

fn reset_register(reg: periph::reg::Reg) {
    reg.write(MY_DEFAULT_VALUE.clone());
}

Then we'd need to sort out the initialization of this new struct. There are three currently-supported cases to consider:

• write(), which writes the "reset value" to any field not specified
• write_with_zero(), which writes the Default (zero for all real usages) to a field not specified
• modify(), which performs a read-modify-write cycle.

How about:

let mut new_values_over_reset = periph.reg.reset();
let mut new_values_over_zero = Default::default();
let mut new_values_over_current = periph.reg.modify();

So now you can more clearly express some useful ideas:

Reset a register:

periph.reg.write(periph.reg.reset());
// instead of
periph.reg.write(|w| w);

Zero a register:

periph.reg.write(Default::default());
// instead of
periph.reg.write_with_zero(|w| w);

Restore a register to a previous value:

let previous_value = periph.reg.modify();

// do some nefarious things with the register

// later...

periph.reg.write(previous_value);

Apologies for the long-winded API exploration. Hopefully this gets people's creative juices flowing!

[burrbull]

I like @David-OConnor's suggestion of stepping through fields directly rather than with an accessor function. I'm not sure if it's feasible, but it would be very nice so I'm using that in these code fragments.

Rust unlike C doesn't support bit-fields, so it is impossible for now.

As others have mentioned, it's perhaps less common to accumulate multiple field changes to apply at once than it is to just tweak a single field. On that assumption, we should design the API to favor the more common case.

It looks like you don't understand how microcontrollers work. Collecting fields in 1 closure is needed to make compiller use 1 volatile read/write operation. As in general mc can read/write only full register.

Large initialization blocks become more uniform & play nicely with rustfmt.

periph.reg.write(|w| { w.field1().bits(newfield1bits); w.field2().set_bit(); w.field3().variant(VARIANT); });

This looks good, but we should investigate how compiller optimizes this code in release and debug mode. If it produces same assembly as chain variant, I'll approve it.

[adamgreig]

Your idea in Version 1 of changing the variant methods to return a unit type so that existing code that doesn't return the write proxy still works without extra semicolons is really nice, I think that could give us a lot of the backwards compatibility and simplicity for single field writes that we'd like while still allowing the nicer syntax for multi-field updates, and removing the API ambiguity and rustfmt sadness.

As @burrbull mentions we'd need to be sure it still optimises to a single operation where possible, but I expect it will.

I think your option 2 is much more radical, which is not to say it's bad but it probably needs a stronger proof of concept to show that it can work. I wouldn't discount the use of multiple field updates either; often it's absolutely required for correct functionality and in other cases just a lot more efficient, so I think they still need to be supported to the same level as single-field updates. Once you open the design space up like this there's a lot of other options - see some recent discussion in https://github.com/stm32-rs/meta/issues/4 for example:

UART[1].cfgr  |= UART_CFGR_PARITY_NONE | UART_CFGR_BITS_8;

is pretty wild, or there's the existing bobbin-rs, tock-os, stm32ral, regen, and some other experiments in what this sort of API could look like.

modify_reg!(gpio, gpioa, MODER, MODER1: Input, MODER2: Output, MODER3: Input);
gpioa_regs.moder.modify(MODER::MODER1::SET);
UCSR0C::set_value(UCSR0C::UCSZ01 | UCSR0C::UCSZ00);

[couchand]

@adamgreig, thanks for pointing me to those other places folks are considering related issues, I've got a lot to read there.

Your idea in Version 1 of changing the variant methods...

Yes, I agree that's probably the best course of action. (Since more daring suggestions were brought up by others I couldn't help myself :))

we'd need to be sure it still optimises to a single operation where possible

Regarding these concerns, it's worth noting that if it doesn't optimize to the same code there's still API design work to be done, because the non-chained version is possible today (and, indeed, common in real-world use). In a quick spot-check, this pattern does seem to optimize to a single operation, so I think it's a non-issue.

---

@burrbull, as to your comment

It looks like you don't understand how microcontrollers work.

I would just ask that you take a few minutes to read the code of conduct and then perhaps see if my proposal already takes into account your concern.

[bradleyharden]

If we're considering significant changes to the API, can I propose a change to a different aspect?

Right now, only the top-level PAC structs are zero-sized. The zero-sized types implement Deref and point to sized RegisterBlocks. The ability to move these zero-sized structs out of the Peripherals struct is convenient, and it allows you to control access to each peripheral. However, that access control only extends to entire peripherals. Alternatively, it would be really nice if every individual register were a zero-sized struct that could be moved. I think that would allow really fined-grained control at the HAL level. Does that sound useful to anyone else? I think there's probably even a way to make it backwards compatible, if we decide against any major API changes.

[burrbull]

If we're considering significant changes to the API, can I propose a change to a different aspect?

Right now, only the top-level PAC structs are zero-sized. The zero-sized types implement Deref and point to sized RegisterBlocks. The ability to move these zero-sized structs out of the Peripherals struct is convenient, and it allows you to control access to each peripheral. However, that access control only extends to entire peripherals. Alternatively, it would be really nice if every individual register were a zero-sized struct that could be moved. I think that would allow really fined-grained control at the HAL level. Does that sound useful to anyone else? I think there's probably even a way to make it backwards compatible, if we decide against any major API changes.

I think this is related to #213

[adamgreig]

We discussed this in the meeting today (logs here); generally I would say a number of people were concerned that the widespread code breakage is not justified by the improvement in clarity, but it was not unanimous. We could go for an RFC for a specific change (perhaps Version 1 from https://github.com/rust-embedded/svd2rust/issues/478#issuecomment-706290264), and have the tools team vote on it after discussion, or we could continue discussion here for a while longer to refine the idea.

[therealprof]

For @therealprof and the rest of the team, here is my attempt at summarizing the main decision points. I will follow with my own personal opinion in a later comment.

@couchand Thanks for your extensive writeup which raises some interesting points.

1. can we, should we, and how would we make breaking changes to generated code?
2. is the register mutation design sufficiently imperfect to justify breaking changes?
3. what shape should the target API for register mutation take?

I'm not opposed to making breaking changes but to me the minor reduction in cognitive complexity doesn't seem to outweigh the downside of breaking the world (and I do mean that quite literally as it does break every HAL impl and every application manipulating registers using the same way -- which are a lot). I feel we might want to aim a bit higher than that if we want to entertain such breakage.

Also some points which didn't seem to receive much consideration so far:

• Compile time speed: This is one of the major drawbacks of the current approach and the suggested changes are most likely not affect this in any way which is not ideal.
• Generated code: The code we're generating right now is okay (with optimisation) and truly abysmal (without optimisation). Any API change will need to be regression tested in that regard and ideally be a improvement for the dev case.

[adamgreig]

If anyone has any more feedback on this issue, please could you leave a comment in the next week? If there's no more updates by the next meeting (10th Nov), I think we should close this issue for now and reconsider the API change alongside any future breaking changes, rather than as a standalone change.

[bradleyharden]

I agree that the benefits to this change are minor and probably don't justify breaking changes. However, I think it would be nice to make this change along with the proposal in #213. I strongly support that proposal.

[couchand]

From my perspective, the biggest concern is that there isn't a clear path or policy regarding making improvements to the generated API. Many on this thread (and elsewhere) have raised interesting suggested directions for this library. However, there doesn't seem to be a sense that any of them have a chance to be incorporated, nor is there clear leadership on the long-term direction here. These factors make me, and likely others, not want to invest much effort in exploring the possible incremental improvements. It seems like this situation makes it very likely that there will be ecosystem fragmentation in the future, as exploration happens elsewhere and isn't incorporated back here. That would be a real shame, but with this library seemingly calcifying due to the weight of unmanaged dependents, it seems unavoidable.

[David-OConnor]

I agree with couchand's sentiments. I think the path to change, if it happens will be like this: Someone or a small cohesive group will make something. If it provides enough advantages, more people will adopt it.

[therealprof]

However, there doesn't seem to be a sense that any of them have a chance to be incorporated,

I'm not quite sure how you arrived at such a dire conclusion. A lot of your improvements have been embraced and incorporated into the project. It would seem very much expected to me that a proposal suffixed with (big breaking change) is not something that would just accepted as is but require warrant some discussion and may even be rejected.

nor is there clear leadership on the long-term direction here.

That's probably fair although not exactly true. We tried to discuss it at our weekly meeting (every Tuesday 8pm CET on Element.io nee Matrix) but no one was there to discuss it. The matter of fact is that we cannot simply break the whole API now since a lot of projects depend on it, especially this is not unequivocally seen as a huge benefit to warrant breaking the world.

Long-term we're planning on completely restructuring the API and could consider such a change. However there're currently no/few proposals on the table on what else we might want to change so that's on the backburner. The general focus is certainly to arrive at something that is a lot quicker to compile and doesn't incur such a nasty performance hit in dev mode binaries. At the moment the compiler still lacks some primitives which could take us there although with the recent bump in const support we might get a lot closer...

[couchand]

My apologies for being a bit fatalistic. You're right that I've had a few changes merged recently, and I appreciate the time and effort that folks took to review and provide feedback! My point was more that, in the absence of a clear strategy for approaching broader changes, such improvements tend to hew very incremental. That's fine (to a point) -- it's important for things to be stable for users.

Long-term we're planning on completely restructuring the API...

Is there a centralized place where this discussion and planning is happening?

[therealprof]

Is there a centralized place where this discussion and planning is happening?

No(t yet). We did brainstorm a bit during the last and after the meeting.

[adamgreig]

The logs from the most recent meeting are here; the meetings are in the usual Embedded Rust Matrix chat/IRC channel (Tuesdays 8pm Berlin time) and open for anyone to join in. I would summarise the general opinion from the meeting as "we know svd2rust probably needs big changes, so let's hold off on API-breaking changes until we work out what they are", but there's no concrete direction yet, and as @David-OConnor suggested, that might take the form of a whole new project/fork which can be evaluated and then perhaps replace svd2rust.

[therealprof]

Removing nominated flag since there wasn't any progress on the discussion.

[David-OConnor]

I agree. Status?