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

Enable SPI2 on subset of stm32l0x1 devices #221

Merged
merged 2 commits into from
Nov 28, 2022

Conversation

jcard0na
Copy link
Contributor

@jcard0na jcard0na commented Nov 23, 2022

There are members of the stm32l0x1 subfamily that thave two SPIs. In
particular the STM32l051 and the STM32l071.

Compile tested as:

cargo build --release --example spi2 --features mcu-STM32L051C8Tx

(builds)

cargo build --release --example spi2  --features mcu-STM32L031C6Tx

(fails to build, as this subfamily only has 1 SPI)

but not run on actual hardware.

@jcard0na jcard0na requested a review from a team as a code owner November 23, 2022 23:56
Copy link
Contributor

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

According to RM0377 Table 2, "Overview of features per category" (ref Table 1 above it), the L011 and L031 don't have SPI2, so I don't think it's the right thing to remove the feature gates completely.

Thankfully it looks like the categories mentioned in that table are mirrored in these somewhat confusingly named features so you could try using those instead. I'd want to diff the cargo doc output to check for any regressions in other parts if you do that though.

examples/spi2.rs Show resolved Hide resolved
src/rtc.rs Show resolved Hide resolved
examples/spi2.rs Outdated Show resolved Hide resolved
examples/spi2.rs Outdated Show resolved Hide resolved
@jcard0na jcard0na force-pushed the upstream branch 2 times, most recently from e109197 to fa6e250 Compare November 24, 2022 17:10
@jcard0na jcard0na changed the title Remove feature guard around SPI2 for stm32l0x1 Enable SPI2 on subset of stm32l0x1 devices Nov 24, 2022
@jcard0na jcard0na force-pushed the upstream branch 3 times, most recently from f0b3bb4 to 0ede113 Compare November 24, 2022 17:41
@jcard0na
Copy link
Contributor Author

Thanks for the quick review. I think I addressed all the comments except that I don't know how to review changes to cargo doc. Can you help with that?

@jamwaffles
Copy link
Contributor

Thanks for the quick review.

Any time!

It looks like there are some remaining NaiveDate errors to sort out :(. Are you able to access the Github Actions logs on this PR? Either way, you should be able to run the commands in .github\workflows\ci.yaml to test for issues on your dev machine.

I don't know how to review changes to cargo doc

I'm not sure what you mean by this I'm afraid. I see warnings when I run cargo doc --features 'rt mcu-STM32L011D3Px' but nothing that stops a build, unless I try cargo doc --release --examples --features 'rt mcu-STM32L011D3Px' which vomits over a conflicting panic dependency. Do you mean these or is there something else coming up?

@jcard0na
Copy link
Contributor Author

It looks like there are some remaining NaiveDate errors to sort out :(. Are you able to access the Github Actions logs on this PR? Either way, you should be able to run the commands in .github\workflows\ci.yaml to test for issues on your dev machine.

Duh. I did replace all the deprecated functions... but I introduced a new call to and_hms(). Fixed now.

I don't know how to review changes to cargo doc

I'm not sure what you mean by this I'm afraid. I see warnings when I run cargo doc --features 'rt mcu-STM32L011D3Px' but nothing that stops a build, unless I try cargo doc --release --examples --features 'rt mcu-STM32L011D3Px' which vomits over a conflicting panic dependency. Do you mean these or is there something else coming up?

I was trying to address your original cargo doc comment:

I'd want to diff the cargo doc output to check for any regressions in other parts if you do that though.

@jcard0na
Copy link
Contributor Author

jcard0na commented Nov 25, 2022

I do get an error when running:

cargo doc --release --examples --features 'rt mcu-STM32L011D3Px'

but the same error shows up before my changes:

warning: build failed, waiting for other jobs to finish...
error: duplicate lang item in crate `panic_semihosting`: `panic_impl`.
  |
  = note: the lang item is first defined in crate `panic_halt` (which `timer_interrupt_rtic` depends on)
  = note: first definition in `panic_halt` loaded from /opt/javier/dev/stm32l0xx-hal/target/thumbv6m-none-eabi/release/deps/libpanic_halt-93ea9eb1b94cbf3a.rmeta
  = note: second definition in `panic_semihosting` loaded from /opt/javier/dev/stm32l0xx-hal/target/thumbv6m-none-eabi/release/deps/libpanic_semihosting-aeee2f7c58ae038e.rmeta

I can complete the build if I comment out all extern crate panic_halt declarations.

@jcard0na jcard0na requested a review from jamwaffles November 25, 2022 15:01
@jamwaffles
Copy link
Contributor

Right, that makes more sense now. It should work if you remove the --examples arg - those don't generate any docs.

If you wanted to fix the root cause, it would be great to have every example use panic_halt and remove panic_semihosting completely. Would also be a good opportunity to replace extern crate panic_halt with use panic_halt as _; which is a bit more modern. I'm happy to hack on this at a later date if you just want to land the SPI2 stuff for now :)

There are members of the stm32l0x1 subfamily that thave two SPIs.  In
particular the STM32l051 and the STM32l071.

Compile tested as:

cargo build --release --example spi2 --features mcu-STM32L051C8Tx
(builds)

cargo build --release --example spi2  --features mcu-STM32L031C6Tx
(fails to build, as this subfamily only has 1 SPI)

but not run on actual hardware.
and_hms() -> and_hms_opt()
from_ymd() -> from_ymd_opt()
@jcard0na
Copy link
Contributor Author

I did try

to have every example use panic_halt and remove panic_semihosting completely.

but that still failed with the error

error: duplicate lang item in crate `panic_semihosting`: `panic_impl`.

Here is the diff in case you want to check it out panic_halt.diff.txt

Given that I don't really know where this conflicting dependency comes from, I think I'll accept your offer to land the SPI2 fix now. Thanks!

Copy link
Contributor

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Given that I don't really know where this conflicting dependency comes from, I think I'll accept your offer to land the SPI2 fix now.

Sure thing, and sorry the fix wasn't as easy as it should be :(. I'll put a follow up PR together to clean that stuff up later. Thanks for the attempt at least!

PR looks good too :)

@jamwaffles jamwaffles merged commit e5be0d8 into stm32-rs:master Nov 28, 2022
jcard0na added a commit to jcard0na/stm32l0xx-hal that referenced this pull request Dec 13, 2022
* Enable SPI2 on subset of stm32l0x1 devices

There are members of the stm32l0x1 subfamily that thave two SPIs.  In
particular the STM32l051 and the STM32l071.

Compile tested as:

cargo build --release --example spi2 --features mcu-STM32L051C8Tx
(builds)

cargo build --release --example spi2  --features mcu-STM32L031C6Tx
(fails to build, as this subfamily only has 1 SPI)

but not run on actual hardware.

* Migrate deprecated NaiveDate functions

and_hms() -> and_hms_opt()
from_ymd() -> from_ymd_opt()

Co-authored-by: Javier Cardona <javier.cardona@gmail.com>
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.

2 participants