[#80] Baudrate type implementation #88

explicite · 2020-10-11T20:47:58Z

Base impl. for discussion

Rahix · 2020-10-13T12:23:19Z

My suggestion would be to start working on this for one board only (preferably some hardware you have and can test things on). Once discussion about the design has settled and all problems are ironed out, it can be transferred to all other crates as well. This way, you don't have to make each change for all the crates every time.

avr-hal-generic/src/serial.rs

explicite · 2020-10-13T20:37:29Z

@Rahix tested default on arduino leonardo. Do you have any specific scenarios for tests?
Right now Baudrate will contain some logic which we test with some units. Do we have any rules for this? Should I define some unit tests?

Rahix · 2020-10-14T08:56:41Z

@explicite, my suggestion was to remove the changes from all board code except arduino-leonardo and its example for now. This way the MR doesn't contain so many changes for now and once everything is looking good, you can readd the changes for the other boards. Otherwise you'll have to update each file again and again which is a bit cumbersome ...

Do you have any specific scenarios for tests?

We should test all the well known baudrates, ideally also with different core clock speeds. Not sure how to easily try other clock speeds though (maybe by setting the clock prescaler?).

Should I define some unit tests?

I guess this would be the right thing to do but we don't really have a testing infrastructure yet ... I think manual testing is enough for now.

avr-hal-generic/src/serial.rs

boards/arduino-leonardo/src/lib.rs

avr-hal-generic/src/serial.rs

Rahix · 2020-10-16T09:53:11Z

avr-hal-generic/src/serial.rs

+        let mut baud_setting = (CLOCK::FREQ / 4 / baud - 1) / 2;
+        let mut u2x = true;
+
+        // Switch back from u2x if required
+        if ((CLOCK::FREQ == 16_000_000) && (baud == 57_600)) || (baud_setting > 4095) {
+            u2x = false;
+            baud_setting = (CLOCK::FREQ / 8 / baud - 1) / 2;
+        }


This is what Arduino is doing but especially that workaround for their bootloader seems suspicious to me. We have to remember that not everyone using the HAL is using it for Arduino boards so we should be careful about shipping Arduino workarounds everywhere by default. I'll look deeper into this and share my results.

In general, enabling U2X consistently yields clock rates with equal or less error than with disabled U2X so I'd say it should be the default where possible. I.e. it makes sense to ship this algorithm in the Baudrate::new() method.

The workaround ((CLOCK::FREQ == 16_000_000) && (baud == 57_600))) is really not something we want to include by default, however. It actually makes the clock error worse, going from -0.8% (with u2x) to +2.1% (without u2x). This is (apparently) necessary because the USB-to-Serial bridge on some arduino boards has a firmware with a similar clock error and the -0.8% in the other direction would make the total error 3% which is too much, I guess.

My suggestion would be to add a custom BaudrateArduinoExt trait where the fix is applied. On the affected boards, BaudrateArduinoExt is pulled into the prelude instead of BaudrateExt.

pub trait BaudrateArduinoExt { fn into_baudrate<CLOCK: crate::clock::Clock>(self) -> Baudrate<CLOCK>; } impl BaudrateArduinoExt for u32 { fn into_baudrate<CLOCK: crate::clock::Clock>(self) -> Baudrate<CLOCK> { let br = Baudrate::new(self); // hardcoded exception for 57600 for compatibility with the bootloader // shipped with the Duemilanove and previous boards and the firmware // on the 8U2 on the Uno and Mega 2560. // // https://github.com/arduino/ArduinoCore-avr/blob/3055c1efa3c6980c864f661e6c8cc5d5ac773af4/cores/arduino/HardwareSerial.cpp#L123-L132 if CLOCK::FREQ == 16_000_000 && br.ubrr == 34 && br.u2x { // (CLOCK::FREQ / 8 / 57600 - 1) / 2 == 16 Baudrate::with_exact(false, 16) } else { br } } }

The only problem I see with this is that it's a bit "magical" and makes it quite non-obvious which version is used. Though the alternative of requiring board users to explicitly do something different depending on the hardware they are using, is against the idea of having a HAL in the first place ...

I done this. Now I'm testing leonardo-serial example. After upload, arduino is not recognized until I will reset. After that I need to cleanup board to works fine without reset.

I'm not sure if it's regression, serial example (need to spend some more time on example itself) or problem with .elf

Right, you're probably trying to access the serial console directly via the USB cable which you use to flash code to the board? That's not going to work on Leonardo as it is different from the other Arduino boards: On other Arduinos there is a separate USB-to-Serial converter between the USB port and the actual MCU. On Leonardo, the USB port is directly connected to the MCU (ATmega32U4) and for Arduino C code, the MCU directly presents itself to the host as a USB-to-Serial converter. In Rust we don't do that (at this time at least). Instead, what you're working with right now is the real USART peripheral connected to the pins D0 (RX) and D1 (TX). So to access it you'll need a separate USB-to-Serial converter to connect to those pins.

Nice, I have some usb to ttl from my old ESP project. After quick test I was able to send some chars with let baudrate = 9600_u32.into_baudrate(); and receive correct response: Will test below combination - with Ext. impl and with exact ubrr

baund UBRR tested

300 767 ok

600 383 ok

1200 191 ok

2400 95 ok

4800 47 ok

9600 23 ok

14400 15 failed

19200 11 ok

28800 7 failed

38400 5 ok

57600 3 ok

76800 2 failed

115200 1 ok

That aside, we should also run a test on an Arduino Uno with the special implementation to check whether that's working as intended. If you don't have a device on hand, I can take care of that :)

I will be able to test with uno in the end of this week. So if with your help we can be faster.

PL-2303HX should support some of the bauds. http://www.prolific.com.tw/UserFiles/files/ds_pl2303HXD_v1_4_4.pdf
Not sure about linux drivers. Right now I'm on 5.4.0-52-generic and I can see it. Not sure if supports all bauds.

Linux will just pass the baudrate to the CDC-ACM endpoint exposed by your PL-2303HX. What the device makes of it is the important part, some just select the closest baudrate, others properly configure their clock for the exact selected speed.

From the datasheet you linked, at least 14400 and 28800 should work though ... You don't by any chance have an oscilloscope at hand to check the signal? I'm a bit short on time this and the next week. Though after that I could run some intrusive tests to see.

That said, I wouldn't block this PR's progress on these few baudrates though. As long as all common rates are well supported, we are good I'd say.

I do not have any oscilloscope now. With time I will buy some. I don't want to invest to much now.

Let's open an issue for this an resolve it at a later time: #99

avr-hal-generic/src/serial.rs

Rahix

From the technical side, I think we're all good now! I've left a few comments related to cleanup, please take care of them. Afterwards, let's do some more tests to make sure it's working correctly now and then fixup all the other HAL & Board crates where necessary. Thanks for working on this :)

avr-hal-generic/src/serial.rs

boards/arduino-leonardo/examples/leonardo-adc.rs

boards/arduino-leonardo/examples/leonardo-i2cdetect.rs

boards/arduino-leonardo/src/lib.rs

chips/atmega32u4-hal/src/lib.rs

Rahix

I have some more minor style nit-picks, but apart from that it's looking really good!

Once you've got those applied, please go ahead and apply the same changes as in boards/arduino-leonardo/src/lib.rs to all the other board crates. We'll have to check which ones need the fixed ..Ext trait and which don't. AFAIK at least arduino-uno and arduino-diecimila will need it. Then, of course all the examples need an update as well.

boards/arduino-leonardo/examples/leonardo-serial.rs

boards/arduino-leonardo/src/lib.rs

chips/atmega32u4-hal/src/lib.rs

boards/arduino-leonardo/src/lib.rs

Rahix · 2020-10-28T21:03:06Z

Oh, and feel free to remove the "Draft" state from this PR :)

explicite · 2020-10-29T19:34:09Z

@Rahix should I implement same changes for other chips and boards?

Rahix

Yeah, I think it looks good now! Please do the changes from boards/arduino-leonardo/src/lib.rs for all the other boards as well now. That means, apply the following changeset:

@@ -54,7 +54,11 @@ pub use crate::pac::Peripherals;
 mod pins;
 pub use crate::pins::*;
 
-pub use crate::hal::prelude;
+pub mod prelude {
+    pub use crate::hal::prelude::*;
+    pub use crate::hal::usart::BaudrateExt as _;
+}
+pub use crate::hal::usart;
 
 /// Busy-Delay
 ///
@@ -187,7 +191,7 @@ pub mod pwm {
 ///     dp.USART1,
 ///     pins.d0,
 ///     pins.d1.into_output(&mut pins.ddr),
-///     57600,
+///     57600.into_baudrate(),
 /// );
 ///
 /// ufmt::uwriteln!(&mut serial, "Hello from Arduino!\r").void_unwrap();

Of course you'll also have to fix all the relevant examples alongside it:

     let mut serial = <board>::Serial::new(
         dp.USART1,
         pins.d0,
         pins.d1.into_output(&mut pins.ddr),
-        57600,
+        57600.into_baudrate(),
     );

explicite · 2020-10-30T20:37:45Z

@Rahix I think all fixed as required. Should I made any changes/testing?

Rahix

Looking pretty good! Only thing we need to change is making use of the special implementation on affected boards. We should test where it applies, I've marked the potential candidates with comments:

boards/arduino-uno/src/lib.rs

Rahix · 2020-10-31T10:01:37Z

boards/arduino-mega2560/src/lib.rs

-pub use crate::hal::prelude;
+pub mod prelude {
+    pub use crate::hal::prelude::*;
+    pub use crate::hal::usart::BaudrateExt as _;


Also need to check whether this is affected by the special implementation. @couchand, maybe you can give a try of this PR at 57600 baud?

In worst case I think will be able to test this in next week.

For mega2560 same results. I will check now Uno rev3 for same baunds

baund UBRR tested

300 767 ok

600 383 ok

1200 191 ok

2400 95 ok

4800 47 ok

9600 23 ok

14400 15 failed

19200 11 ok

28800 7 failed

38400 5 ok

57600 3 ok

76800 2 failed

115200 1 ok

This is with the BaudrateExt trait, right? So it seems 57600 and 115200 are working fine with the default implementation which means we don't need to switch for this board!

Yes. Looks like works fine. I'm able to test today uno rev3 too. Then I will need to reupload bootloader for mega2560...

If you don't have any objections, I'm content with the results you provided and would go ahead with merging your PR now. Thank you so much for you work here! You were a lot more thorough than most people would be which I appreciate a lot. :)

boards/arduino-diecimila/src/lib.rs

Rahix · 2020-11-06T11:27:49Z

Looks like you'll need to fix merge conflicts/rebase your branch once more, then I can merge it :)

Rahix added the hal-api API design for the different components of avr-hal label Oct 13, 2020

Rahix reviewed Oct 13, 2020

View reviewed changes

avr-hal-generic/src/serial.rs Show resolved Hide resolved

avr-hal-generic/src/serial.rs Show resolved Hide resolved

Rahix reviewed Oct 16, 2020

View reviewed changes

Rahix changed the title ~~[#80] Baundrate type implementation~~ [#80] Baudrate type implementation Oct 20, 2020

Rahix reviewed Oct 25, 2020

View reviewed changes

avr-hal-generic/src/serial.rs Outdated Show resolved Hide resolved

Rahix reviewed Oct 28, 2020

View reviewed changes

Rahix requested changes Oct 28, 2020

View reviewed changes

explicite marked this pull request as ready for review October 29, 2020 19:33

Rahix approved these changes Oct 30, 2020

View reviewed changes

Rahix requested changes Oct 31, 2020

View reviewed changes

explicite mentioned this pull request Oct 31, 2020

Arduino Nano Every support #97

Open

Rahix mentioned this pull request Nov 6, 2020

Some uncommon baudrates might be unstable #99

Closed

Baudrate type implementation Rahix#88

64c152f

explicite force-pushed the topic-80 branch from 237c488 to 64c152f Compare November 6, 2020 20:28

Merge remote-tracking branch 'upstream/master' into topic-80

c660ae2

Rahix approved these changes Nov 6, 2020

View reviewed changes

Rahix merged commit 4c0094a into Rahix:master Nov 7, 2020

explicite deleted the topic-80 branch November 9, 2020 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#80] Baudrate type implementation #88

[#80] Baudrate type implementation #88

explicite commented Oct 11, 2020

Rahix commented Oct 13, 2020

explicite commented Oct 13, 2020

Rahix commented Oct 14, 2020

Rahix Oct 16, 2020

Rahix Oct 20, 2020

explicite Oct 24, 2020

Rahix Oct 24, 2020

explicite Oct 25, 2020 •

edited

Loading

Rahix Oct 26, 2020

explicite Oct 26, 2020

Rahix Oct 26, 2020

explicite Nov 5, 2020 •

edited

Loading

Rahix Nov 6, 2020

Rahix left a comment

Rahix left a comment

Rahix commented Oct 28, 2020

explicite commented Oct 29, 2020

Rahix left a comment

explicite commented Oct 30, 2020

Rahix left a comment

Rahix Oct 31, 2020

explicite Oct 31, 2020

explicite Nov 5, 2020

Rahix Nov 6, 2020

explicite Nov 6, 2020

Rahix Nov 6, 2020

Rahix commented Nov 6, 2020

baund	UBRR	tested
300	767	ok
600	383	ok
1200	191	ok
2400	95	ok
4800	47	ok
9600	23	ok
14400	15	failed
19200	11	ok
28800	7	failed
38400	5	ok
57600	3	ok
76800	2	failed
115200	1	ok

[#80] Baudrate type implementation #88

[#80] Baudrate type implementation #88

Conversation

explicite commented Oct 11, 2020

Rahix commented Oct 13, 2020

explicite commented Oct 13, 2020

Rahix commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

explicite Oct 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

explicite Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rahix left a comment

Choose a reason for hiding this comment

Rahix left a comment

Choose a reason for hiding this comment

Rahix commented Oct 28, 2020

explicite commented Oct 29, 2020

Rahix left a comment

Choose a reason for hiding this comment

explicite commented Oct 30, 2020

Rahix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rahix commented Nov 6, 2020

explicite Oct 25, 2020 •

edited

Loading

explicite Nov 5, 2020 •

edited

Loading