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.

Sign up for GitHub

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

Jump to bottom

Serial: Replace Pins trait with TxPin / RxPin #68

Merged
merged 3 commits into from
Apr 4, 2020
Merged

Serial: Replace Pins trait with TxPin / RxPin #68

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.

dbrgn
dbrgn commented Jan 7, 2020
View reviewed changes
examples/rtc.rs Show resolved Hide resolved
@arkorobotics arkorobotics mentioned this pull request Jan 10, 2020
Closed
@dbrgn dbrgn force-pushed the serial-tx-rx-traits branch from 0357eae to 632955f Compare January 10, 2020 16:59
@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 dbrgn force-pushed the serial-tx-rx-traits branch from 632955f to 1b0837a Compare January 13, 2020 17:35
@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 13, 2020 •
edited
Loading

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?

@dbrgn dbrgn mentioned this pull request Jan 17, 2020
Closed
@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.

rnestler
rnestler approved these changes Jan 21, 2020
View reviewed changes
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.

@dbrgn dbrgn mentioned this pull request Feb 23, 2020
Merged
@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.

@dbrgn
Serial: Replace Pins trait with TxPin / RxPin
d408ae7 
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.
@dbrgn dbrgn mentioned this pull request Apr 3, 2020
Closed
@dbrgn dbrgn force-pushed the serial-tx-rx-traits branch from 1b0837a to 9321f0b Compare April 3, 2020 22:43
@dbrgn
Serial: Implement UART peripherals based on device category
da1edcd 
This requires setting an io-* Cargo feature in order to access the
`serial` module.
@dbrgn dbrgn force-pushed the serial-tx-rx-traits branch from 9321f0b to da1edcd Compare April 3, 2020 23:06
@dbrgn
Serial: Impl pins based on io-* features
26bbfcf 
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.

@dbrgn dbrgn mentioned this pull request Apr 3, 2020
Merged
@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.

@arkorobotics arkorobotics merged commit 4ed1629 into stm32-rs:master Apr 4, 2020
@arkorobotics arkorobotics mentioned this pull request Apr 4, 2020
Merged
@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
This was referenced Apr 6, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hannobraun hannobraun hannobraun left review comments

@rnestler rnestler rnestler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Serial Pins design discussion
4 participants
@dbrgn @arkorobotics @hannobraun @rnestler