-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop return value from register accessor closure type (big breaking change) #478
Comments
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. |
Nominating this for a broader discussion in the next meeting. |
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
If this PR isn't possible to implement, I think a doc change making clear that the |
I mostly use PACs directly rather than writing HALs, but in my experience almost all uses of 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 |
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:
This doesn't play nicely with rustfmt, which re-layouts this to something like:
... 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? |
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 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(); |
@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 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 |
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. |
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? |
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. |
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:
Breaking changes in the generated codeProblem statement: There's not a clear versioning story for this tool.
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 APIProblem 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:
Ideal design for the APIProblem 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:
There are a few important considerations to take into account:
|
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 codeIt'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:
Concerns with the current APIThe 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 APII approach this from two sides:
Version 1: minimal changesI 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:
fn write<F>(&self, f: F) where F: FnOnce(&mut W);
periph.reg.write(|w| {
w.field1().bits(newfield1bits);
w.field2().set_bit();
w.field3().variant(VARIANT);
});
reg.modify(|_, w| w.field().variant()); Version 2: total redesignOn the other hand, if I were designing this API from scratch per (2) I might do things differently.
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 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 // 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 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:
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! |
Rust unlike C doesn't support bit-fields, so it is impossible for now.
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.
periph.reg.write(|w| { 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. |
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 stm32-rs/meta#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); |
@adamgreig, thanks for pointing me to those other places folks are considering related issues, I've got a lot to read there.
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 :))
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
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. |
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 |
I think this is related to #213 |
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 #478 (comment)), and have the tools team vote on it after discussion, or we could continue discussion here for a while longer to refine the idea. |
@couchand Thanks for your extensive writeup which raises some interesting points.
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:
|
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. |
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. |
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. |
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. |
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.
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 |
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.
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. |
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. |
Removing nominated flag since there wasn't any progress on the discussion. |
I agree. Status? |
632: doc examples for write r=Emilgardis a=burrbull Related #478 Co-authored-by: Andrey Zgarbul <[email protected]>
The methods
Reg::write
,Reg::write_with_zero
, andReg::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
svd2rust/src/generate/generic.rs
Lines 95 to 99 in 29129c0
However, the ramification of this design on real-world code is not as nice. See, for instance, in
stm32-rs
oravr-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:or
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:
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.
The text was updated successfully, but these errors were encountered: