Hi Fiona,
Thanks for this! I noticed a procedural thing (the last diff chunk,
please see below), so I took the chance to give a few quick
surface-level comments I noticed while scrolling, mostly on docs :)
On Mon, Jan 13, 2025 at 1:16 PM Fiona Behrens <me@xxxxxxxxxx> wrote:
>
> + /// Get String representation of the Color.
[`String`], [`Color`]
i.e. please format, and use intra-doc links where possible/reasonable.
> + // SAFETY: pointer from above, valid for static lifetime.
In general, try to explain why, not just what, in the safety comments,
i.e. why it is valid for a static lifetime? e.g. does the C API
guarantee it?
> + /// Get String representation of the Color as rust type.
> + #[inline]
> + pub fn name(&self) -> Option<&'static str> {
"Rust type"
However, I am not sure what it is trying to say. I would have thought
it is a custom newtype of something or perhaps something strange is
going on, but I guess you are referring to `str` vs. `CStr`? In
general, I don't think we need to say "as a Rust type" -- it may
confuse more than help.
> + // SAFETY: name from C name array which is valid UTF-8.
What guarantees this? i.e. I imagine you looked into all the cases
from the static table, so I would for instance say: "All values in the
C name array are valid UTF-8." or similar (perhaps mention which
array, so that people can e.g. grep).
> +impl core::fmt::Display for Color {
> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> + f.write_str(self.name().unwrap_or("Invalid color"))
> + }
> +}
Can this happen? If not, should `Color` have an `# Invariant` and
would we need the `Option<...>` for names then?
In any case, here, shouldn't the error be bubbled up?
> + /// Tris to convert a u32 into a [`Color`].
Typo: Tries
Format: [`u32`]
There are also other typos ("brightnes", "Activae") -- I recommend
`checkpatch.pl` with the `--codespell` flag which may help catching
some of these.
> + /// Get the LED brightness level.
> + fn brightness_get(_this: &mut Self::This) -> u8 {
> + crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
> + }
Please use the macro instead (see 15f2f9313a39 ("rust: use the
`build_error!` macro, not the hidden function")). Soon we will have it
in the prelude too (see 4401565fe92b ("rust: add `build_error!` to the
prelude")), by the way.
> + // SAFETY: By the safety requirement of this function `delay_on` is pointing to a valid `c_ulong`.
> + let on = Delta::from_millis(unsafe { *delay_on } as i64);
I didn't look into most comments, but e.g. I noticed this one does not
look correct -- the safety requirements for this function do not
mention anything about `delay_on`, no?
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 38da79925586..3c348ce4a7c2 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -143,4 +143,11 @@ pub fn as_nanos(self) -> i64 {
> pub fn as_micros_ceil(self) -> i64 {
> self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
> }
> +
> + /// Return the smallest number of milliseconds greater than or equal
> + /// to the value in the `Delta`.
> + #[inline]
> + pub fn as_millis_ceil(self) -> i64 {
> + self.as_nanos().saturating_add(NSEC_PER_MSEC - 1) / NSEC_PER_MSEC
> + }
This should probably be its own patch, with Cc to the timekeeping
maintainers etc.
Cheers,
Miguel
