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

Serial: Replace Pins trait with TxPin / RxPin #68

Merged
merged 3 commits into from
Apr 4, 2020

Conversation

dbrgn
Copy link
Contributor

@dbrgn dbrgn commented Jan 7, 2020

Based on #66, merge that first. Fixes #67.

The previous approach with the Pins trait assumed that an USART
peripheral can only be used with specific pin pairs. However, pins can
be mixed, so you could use USART1 with PA9 and PB7.

So instead of the Pins trait, there are now separate TxPin and
RxPin traits. They are implemented for all pins of all U(S)ART
peripherals of the stm32l0xx family.

Pin mappings verified against datasheets of stm32l071kb, stm32l072vz
and stm32l083vz.

examples/rtc.rs Show resolved Hide resolved
@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 10, 2020

Rebased.

@arkorobotics
Copy link
Member

Per the discussion in #67, this PR looks good to me. @hannobraun thoughts?

@hannobraun
Copy link
Contributor

I've taken a look and it looks good to me. But I didn't check any of this against the data sheets, and don't have time right now.

Pin mappings verified against datasheets of stm32l071kb, stm32l072vz and stm32l083vz.

Are we sure that's good enough? I'm not volunteering, but it certainly wouldn't hurt if someone bit the bullet and checked it against all of them :-)

@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 13, 2020

Rebased again.

Are we sure that's good enough? I'm not volunteering, but it certainly wouldn't hurt if someone bit the bullet and checked it against all of them :-)

Since the previous values were sometimes plain wrong, it should not get any worse 😉 I did double-check all values against the datasheet, however it probably wouldn't hurt if someone else did that too.

My approach was going to the ST page, opening the product selector for each chip family, picking a device with a large pin count package (e.g. LQFP 100) and taking a look at that datasheet.

The values can be checked using the pin definition table, but it's a bit easier using the alternate functions table.

@arkorobotics
Copy link
Member

Do we want to wait for these values to be verified or should we merge and update later as needed?

@rnestler
Copy link
Contributor

I verified with the STM32L010RB (which isn't supported according to the readme). That one is definitely not compatible since it only has USART2 and LPUART1. Also it doesn't have a variant with 100 pins.
But the pin mappings for USART2 an LPUART1 do match.

Copy link
Contributor

@rnestler rnestler left a comment

Choose a reason for hiding this comment

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

I also verified STM32L071RB against the datasheet. For me this is good enough for now.

@dbrgn
Copy link
Contributor Author

dbrgn commented Feb 4, 2020

Is there anything blocking this PR?

@arkorobotics
Copy link
Member

Is there anything blocking this PR?

cc @hannobraun

@hannobraun
Copy link
Contributor

I'm fine with this!

@dbrgn
Copy link
Contributor Author

dbrgn commented Feb 5, 2020

Crap. This is on hold: #76 (comment)

Apparently not all STM32L0x1 are created equal when considering AFs.

@arkorobotics
Copy link
Member

@dbrgn Minor conflicts with the last merge, please advise.

@arkorobotics
Copy link
Member

Not sure how I closed this PR. Reopening for the time being.

@arkorobotics arkorobotics reopened this Feb 25, 2020
@dbrgn
Copy link
Contributor Author

dbrgn commented Feb 25, 2020

Thanks! Now that #82 is merged, I'll proceed to generate proper mappings for the different GPIO peripheral versions.

@dbrgn
Copy link
Contributor Author

dbrgn commented Feb 25, 2020

Alternatively, if #83 were implemented first, that would make things much easier.

The previous approach with the `Pins` trait assumed that an USART
peripheral can only be used with specific pin pairs. However, pins can
be mixed, so you could use USART1 with PA9 and PB7.

So instead of the `Pins` trait, there are now separate `TxPin` and
`RxPin` traits. They are implemented for all pins of all U(S)ART
peripherals of the stm32l0xx family.

Pin mappings verified against datasheets of stm32l071kb, stm32l072vz
and stm32l083vz.
This requires setting an io-* Cargo feature in order to access the
`serial` module.
The previous definitions were based on the io-STM32L071 feature (product
category 5).
@dbrgn
Copy link
Contributor Author

dbrgn commented Apr 3, 2020

@arkorobotics I think I have sorted this out! As suggested in this comment:

Thanks! Now that #82 is merged, I'll proceed to generate proper mappings for the different GPIO peripheral versions.

...I made the peripheral availability and the pin mappings dependent on the io-* features, similar to I²C in #87. See #87 for more details.

The pin mappings used in this changeset were taken out of the cube-parse output for the STM32L0 family. The output can be found here. To re-generate:

cargo run -- pin_mappings STM32L0 -d /path/to/stm32cubemx/db/mcu/

The long-term solution should probably be a universal pin mapping file generated by cube-parse, but with the current APIs this would be a major refactoring. Therefore a manual fix must suffice for now.

Closes #88. Fixes #67.

@arkorobotics this is now ready for review! Maybe @hannobraun would also be willing to take a quick look.

@arkorobotics
Copy link
Member

Nice work! I love the idea of a universal pin mapping file, but as you pointed out it would require major changes. This is great for now :)

Unless @hannobraun has any objects, I'll go ahead and merge this soon.

@hannobraun
Copy link
Contributor

I've only had the time to take a short glance, but no objections from me.

@dbrgn
Copy link
Contributor Author

dbrgn commented Apr 4, 2020

Great, thanks for the review and the merge!

@dbrgn dbrgn deleted the serial-tx-rx-traits branch April 4, 2020 23:12
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.

Serial Pins design discussion
4 participants