Conversation
|
Is there any difference that would justify the duplicated implementation? It doesn't seem right that all PIO based drivers would need such a duplication. What would we need to factorize this code ? |
jannic
left a comment
jannic
left a comment
There was a problem hiding this comment.
Thanks for continuing to update this pull request, even though we didn't react for months.
My comments are only about small improvements, overall I like the approach. Very nice that it's not necessary to duplicate lots of code!
src/lib.rs
Outdated
| /// delay_for_at_least_60_microseconds(); | ||
| /// }; | ||
| ///``` | ||
|
|
There was a problem hiding this comment.
clippy complains about this empty line:
warning: empty line after doc comment
--> src/lib.rs:70:1
|
70 | / ///```
71 | |
| |_^
72 | pub struct Ws2812Direct<P, SM, I>
| ----------------------- the comment documents this struct
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/beta/index.html#empty_line_after_doc_comments
= note: `#[warn(clippy::empty_line_after_doc_comments)]` on by default
= help: if the empty line is unintentional, remove it
src/lib.rs
Outdated
| /// // Do other stuff here... | ||
| /// }; | ||
| ///``` | ||
|
|
There was a problem hiding this comment.
Same here:
warning: empty line after doc comment
--> src/lib.rs:254:1
|
254 | / ///```
255 | |
| |_^
256 | pub struct Ws2812<P, SM, C, I>
| ----------------- the comment documents this struct
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/beta/index.html#empty_line_after_doc_comments
= help: if the empty line is unintentional, remove it
| if #[cfg(feature = "rp2040")] { | ||
| use rp2040_hal as hal; | ||
| } else if #[cfg(feature = "rp235x")] { | ||
| use rp235x_hal as hal; | ||
| } else { | ||
| compile_error!("Either 'rp2040' or 'rp235x' feature must be enabled."); | ||
| } |
There was a problem hiding this comment.
This is a bit confusing for the user: Adding --feature rp235x without also adding --no-default-features doesn't actually enable rp235x support.
I wonder if there's a better alternative. Maybe at least catch the situation where both features are enabled and throw a compile error with a nice explanation? Or make --feature rp235x implicitly disable rp2040 support? (Which could be implemented by switching the order of the two if branches.)
| [features] | ||
| default = ["rp2040"] | ||
| rp2040 = ["dep:rp2040-hal"] | ||
| rp235x = ["dep:rp235x-hal"] |
There was a problem hiding this comment.
I didn't try it and there may be obstacles, but it could be an alternative to select the right dependencies automatically based on the compile target using platform specific dependencies.
rp2040 should always be thumbv6m-none-eabi, and rp235x may be thumbv8m.main-none-eabihf, thumbv8m.main-none-eabi or riscv32imac-unknown-none-elf.
3 participants
Two separate features for rp2040-hal and rp235x-hal