Skip to content
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

Bump rp2040-hal to 0.10.0 and embedded-hal to 1.0 #60

Merged
merged 37 commits into from
Mar 19, 2024

Conversation

martinsp
Copy link
Contributor

@martinsp martinsp commented Mar 16, 2024

Depends on unreleased version of ws2812-pio with rp2040-hal 0.10.0 support

@martinsp
Copy link
Contributor Author

Bumped ws2812-pio to 0.8

Copy link
Member

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR !!!

I'd only request one change (but of course repeated for so many boards :o).

@martinsp martinsp force-pushed the rp2040-hal-update branch from c42bbdf to 5a06d24 Compare March 17, 2024 13:25
@martinsp
Copy link
Contributor Author

I'd only request one change (but of course repeated for so many boards :o).

@ithinuel I have made the changes

Copy link
Member

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last bit is a little nit, sorry I missed that in your previous iteration.

@@ -66,12 +64,12 @@ fn main() -> ! {
);

let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mut can be placed on the first binding :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I change let timer = Timer::new(pac.TIMER, &mut pac.RESETS, &clocks); to let mut timer = Timer::new(pac.TIMER, &mut pac.RESETS, &clocks);

Then compiler complains with the following error:

error[E0502]: cannot borrow `timer` as mutable because it is also borrowed as immutable
  --> boards/adafruit-itsy-bitsy-rp2040/examples/adafruit_itsy_bitsy_rainbow.rs:72:9
   |
63 |         timer.count_down(),
   |         ----- immutable borrow occurs here
...
69 |         ws.write(brightness(once(wheel(n)), 32)).unwrap();
   |         -- immutable borrow later used here
...
72 |         timer.delay_ms(25);
   |         ^^^^^^^^^^^^^^^^^^ mutable borrow occurs here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see. It might be worth adding a note that the "rebind" is to force a copy of the timer.
Or maybe change 63 | timer.count_down(), to 63 | timer.clone().count_down(), ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timer.clone().count_down() fails with an error creates a temporary value which is freed while still in use, so I added a note

@@ -80,12 +78,12 @@ fn main() -> ! {
// Infinite colour wheel loop

let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -62,12 +60,12 @@ fn main() -> ! {
);

let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -66,12 +64,12 @@ fn main() -> ! {
);

let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -80,12 +78,12 @@ fn main() -> ! {
// Infinite colour wheel loop

let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -67,12 +65,12 @@ fn main() -> ! {

// Infinite colour wheel loop
let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -79,12 +77,12 @@ fn main() -> ! {
// Infinite colour wheel loop

let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -78,12 +76,12 @@ fn main() -> ! {
// Infinite colour wheel loop

let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -67,12 +65,12 @@ fn main() -> ! {

// Infinite colour wheel loop
let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -67,12 +65,12 @@ fn main() -> ! {

// Infinite colour wheel loop
let mut n: u8 = 128;
let mut timer = timer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@jannic
Copy link
Member

jannic commented Mar 18, 2024

error: package `embedded-hal-async v1.0.0` cannot be built because it requires rustc 1.75 or newer, while the currently active rustc version is 1.72.0-nightly

I think we need to set MSRV to 1.75, that's also what rp2040-hal uses.

@martinsp
Copy link
Contributor Author

error: package `embedded-hal-async v1.0.0` cannot be built because it requires rustc 1.75 or newer, while the currently active rustc version is 1.72.0-nightly

I think we need to set MSRV to 1.75, that's also what rp2040-hal uses.

Should I do this as part of this PR or this will be dealt separately?

I guess the toolchain version should be aligned in this block

with the one in rp2040-hal ?

@jannic
Copy link
Member

jannic commented Mar 18, 2024

Should I do this as part of this PR or this will be dealt separately?

I think it would be easiest to change it in this PR, otherwise you'd have to rebase this PR to get the pipeline fixed.

I guess the toolchain version should be aligned in this block


with the one in rp2040-hal ?

Yes, replacing toolchain: nightly-2023-06-27 with toolchain: nightly-2024-01-30 should work.

@martinsp
Copy link
Contributor Author

I tested GitHub action on my fork and had to adjust dependencies for pimoroni-tufty204 board to make udeps step pass, looks like it was failing on main branch also

@jannic
Copy link
Member

jannic commented Mar 19, 2024

Looks good! Thank you for the patience to address all our comments. This is a huge step forward towards BSP releases based on rp2040-hal 0.10.0.

@jannic jannic merged commit 3fdd613 into rp-rs:main Mar 19, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants