This is a draft pull request until I test it on Raspberry Pi Pico.

I tested the PR against c_hello and blink applications loaded on Raspberry Pi Pico.

I went through this PR when I looked at the other nonzerou32 pr, and the benefit of this change is less clear to me. A baud rate of 0 doesn't make much sense, sure, but neither does a baud rate of 1 really, or even something like 115201. I don't think including the non zero requirement in the API expresses much that is semantically meaningful.

Also, the simplicity of just using a standard type is nice and simpler to manage. Divide by zero protection is nice, but, in theory any u32 value could become a denominator somewhere; perhaps the check is better placed where the / operator is used, not in the value type. And, requiring new handling (such as calling .unwrap()) might make the potential for panics worse.

A baud rate of 0 doesn't make much sense, sure, but neither does a baud rate of 1 really, or even something like 115201.

Many UART peripherals that I've seen do support baud rates of 1 or 115201 if an appropiate clock divider is used. If you intend to use only some "standard" baud rates in Tock, why don't use an enum for baud rate instead of u32?

Also, the simplicity of just using a standard type is nice and simpler to manage.

Fatal mistake not including NonZero* types as primitive types. Another fatal mistake is defining division by u* instead of NonZero*.

Divide by zero protection is nice, but, in theory any u32 value could become a denominator somewhere; perhaps the check is better placed where the / operator is used, not in the value type.

I don't quite understand what you mean by this? The fact that in some parts of the code, I extracted the raw, inner, non-zero value of NonZeroU32 and used it in several places, including division? This was intentional, to keep the code as close as possible to the original one, otherwise I get comments such as it would be easier to review and approve the PR if it mimicked the original code.

And, requiring new handling (such as calling .unwrap()) might make the potential for panics worse.

Does it? All but two added unwrap()s are in boards crates, one in capsules and one in chips. And all of them are trivial unwrap()s: NonZeroU32::new(115_200).unwrap(). I could in theory use new_unchecked(), but most Rust developers tend to prefer the safe way even if it is obvious the "unsafe" counterpart is in reality safe.

Again, to keep code as similar as possible to the previous version, I did not extract any baud rate as a const value, which would mean that all those "possible" unwrap()s will be catched by the compiler.

I do agree that using NonZeroU32 might seem tedious, especially that the console capsule does not expose a set_baud_rate command to user space. However, using the type system at its entire capacity helps in producing reliable code that's easier to test.

I agree with @Ioan-Cristian's reasoning here. While there are non-zero baudrate values that would raise eyebrows, a baudrate set to zero can never make sense. Tock probably has other potential divide by zero issues; nonetheless I think that we should take every opportunity to avoid them. NonZeroU32, though not as convenient as other integer types, is a fairly standard way of expressing these constraints, can often produce more efficient code & memory representations, and can only make this API more expressive & safe, not less.

We discussed this a bit, and the summary comes out to:

• Reasons in favor of this change
  • It makes a failure/panic case (divide by zero) impossible and the compiler can omit that panic.
  • It encourages using Rust features to eliminate panic sources.
• Reasons opposed to the change
  • Adds churn in the codebase
  • This fixes a rust problem, not a Tock problem.

I think if this touched one or two files it's probably a net improvement, but I'm worried this doesn't really capture what we want (several boards have very specific baud rates they support, not just any non-zero number). Having an associated type where the drivers specify what they can support (including not 0) would, in my opinion, make using the uart in tock easier. But using nonzerou32, while it does eliminate a potential panic, does not really help with Tock as an embedded OS because no one is trying to use a baud rate of 0. I still think it would be a lot simpler to just use a non panicing divide and return on an error.

@Ioan-Cristian One of the questions we had was the main motivation for this PR. The configure method can already return an error, so it'd be fine to turn a zero frequency into this error. .Was it to get rid of panics? In that case, using a checked_div should be fine and will allow this PR be pushed over the finish line quickly.

Adds churn in the codebase

I wouldn't call NonZero::new(115_200).unwrap() churn.

This fixes a rust problem, not a Tock problem.

It's a Rust problem that will never get fixed. Removing division by 0 is a breaking change that affects, unfortunately, many crates. This is why it needs to be solved by Tock itself, even at the expensive of "churn".

I think if this touched one or two files it's probably a net improvement

66 of the touched files are in boards. I changed most of them through a Vim macro. Counting files in this context is meaningless in my opinion. Furthermore, the remaining 14 modified files are minorly impacted.

but I'm worried this doesn't really capture what we want (several boards have very specific baud rates they support, not just any non-zero number). Having an associated type where the drivers specify what they can support (including not 0) would, in my opinion, make using the uart in tock easier.

I'm also worried that a zeroable type doesn't capture the intent of not wanting to pass a 0 baud rate to an UART peripheral driver. Also, shouldn't PRs be (ideally) small atomic changes so they can be reviewed easily? This PR doesn't pose any problems to a future PR adding the baud rate as an associated type with a From<NonZeroU32> trait bound.

no one is trying to use a baud rate of 0

0 is a tricky number. Mathematicians in Antiquity needed several centuries to figure out how to use it. Allowing baud rate to be 0 is analogous to non-nullable C pointers being null, though less harmful.

I still think it would be a lot simpler to just use a non panicing divide and return on an error.

This supposes the benevolence of the programmer to consistently use non-panicking divides. Otherwise, on reviewers' attention to spot when such divides are not used. Not to mention that errors are generated deeper in the kernel. Also, depending on how far one wants to push the argument of simpleness, they can return to C when trying to write performant, low-level code, because it's much simpler.

@Ioan-Cristian One of the questions we had was the main motivation for this PR. The configure method can already return an error, so it'd be fine to turn a zero frequency into this error. .Was it to get rid of panics? In that case, using a checked_div should be fine and will allow this PR be pushed over the finish line quickly.

The main motivation is expressivity through the type system. Rust has a powerful type system, which is actually Turing complete, so I try to take advantage of it as much as possible. By taking advantage of the type system, the programmer can:

1. Better express their intent to other developers => less need for documentation which gets stale overtime + speeds up code comprehension for newcomers
2. Better express their intent to the compiler => more optimized code, more errors detected at compile time, less frustation spent on runtime bugs

Not to mention that it looks there is some interest in certifing Tock for critical domains such as automotive, aeronautics, medical etc. which can take advantage of aggressive use of the type system.

I do agree that for the current state of the UART, such a minor change seems to be an overkill. By I strongly believe that taking the habit of making such changes will eventually lead to a less buggy, more maintenable, more performant, more efficient code.