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

Add more configuration options to UART #63

Merged
merged 2 commits into from
May 28, 2019

Conversation

TheZoq2
Copy link
Member

@TheZoq2 TheZoq2 commented May 4, 2019

This PR adds the the ability to configure the stop bits and parity bits of the serial device. It is inspired by the implementation in the f4 hal but it does differ a bit from that.

The main difference is that this implementation doesn't allow you to specify word length which I did for 2 reasons:

  • Reading/sending 9 bits would require using a u16 instead of u8
  • "Word length" includes parity bits

The second point means that a device configured to send 9 bit words with one parity bit actually sends 8 bit words and the parity bit. This seems to go against the way UART protocols are usually defined. To avoid that confusion, I decided to just disable the option of changing word size and assume the user wants to send 8 bits of actual data.

Another difference to the f4 impl is that I don't use a separate config submodule. To me, that feels unnecessary, but feel free to convince me otherwise.

Finally, this works until we add support for USART5 and 6 which don't allow 1.5 and 0.5 stop bits. Should we future proof this design for that?

Copy link
Member

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

Whoops, totally missed that.

Looks okay to me but could use some cleanup and definitely needs a change log entry with a breaking tag.


let sent = b'U';
block!(tx.write(sent)).ok();
// block!(timer.wait());
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of these commented out lines?

Copy link
Member

Choose a reason for hiding this comment

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

Also if we're not going to use the timer, why initialise it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I'll remove the comments and timer

src/serial.rs Outdated

impl Default for Config {
fn default() -> Config {
let baudrate = 19_200_u32.bps();
Copy link
Member

Choose a reason for hiding this comment

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

19200 baud is kind of an odd choice, I haven't seen that in common use since the age of the acoustic coupler. Maybe either use 9600 or 115200 baud instead which are the two common default baud rates used nowadays (with a slight preference for the latter).

Copy link
Member Author

Choose a reason for hiding this comment

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

You're absolutely right, I have no idea what made me chose 19200. 115200 makes sense to me.

@TheZoq2
Copy link
Member Author

TheZoq2 commented May 28, 2019

I implemented the changes you suggested, does everything look OK now?

Also, the changelog is starting to get big, perhaps we should do a release soon. Though maybe we should fix all the deprecation warnings introduced by embedded_hal 2.2.3 first

Copy link
Member

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks.

@therealprof
Copy link
Member

Release sounds good to me. Not sure we really want to deal with the fall out and actually how to deal with it. I tried changing the stm32f0xx-hal to digital::v2 but failed in a compatible manner without breaking all depending crates.

@therealprof therealprof merged commit 1ba8681 into stm32-rs:master May 28, 2019
@TheZoq2
Copy link
Member Author

TheZoq2 commented May 28, 2019

Yea, that change seems strange to me, especially since it's a minor change. It was very strange to do a cargo clean and discover hundreds of warnings.

I tried updating it by just adding Void return types, but it will, as you say, break all depending crates.

I'll do a release with what we have for now, then we can figure out what to do about this later

Edit: I guess you beat me to doing a release 👍

@TheZoq2
Copy link
Member Author

TheZoq2 commented May 28, 2019

Looks like you forgot to bump the the version in the README, should we push a 0.3.1 to fix that or just leave it? @therealprof

@therealprof
Copy link
Member

Huh, I did? Shouldn't matter too much.

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.

None yet

2 participants