Add a tx only and a rx only serial instance #34

david-sawatzke · 2019-01-05T21:08:16Z

This also makes the serial stuff in #27 obsolete.
Additionally by virtue of how it works also implements stm32-rs/stm32f4xx-hal#49 here.

Not sure how we should handle the release method. It currently works but you get a () for the unused pin back.

Currently untested

HarkonenBade · 2019-01-05T22:18:03Z

I have some thoughts on this, will put together an actual review in a bit, kinda busy atm

src/serial.rs

HarkonenBade · 2019-01-05T22:32:34Z

src/serial.rs

+                    let rxpin = ();
+                    let mut serial = Serial { usart, pins: (txpin, rxpin) };
+                    serial.enable(baud_rate, clocks);
+                    serial


Why not have the Tx only constructor just return a Tx<USART>?

I want to enable listen and potential future things for Rx/Tx only usarts. Returning a Tx would also make release pretty hard to do.

To be fair currently if you split() an interface you already lose the ability to release, so maybe Tx and Rx need to keep hold of their pins and implement release each. Also I could see claim for being able to call listen/unlisten on Tx and Rx instances, or even just make the listening methods on some global type, given they are atomic and not bound to a given object.

That's why stm32-rs/stm32f4xx-hal#49 was done. As a side effect, this is also accomplished by this pull request (an separate object sounds really annoying)

I don't see how that fixes split usarts from being unable to release their pins?

I suppose I tend to see splitting usarts as a useful and important feature as it allows them to be partially shared into interrupt handlers in a sensible way without lock contention.

@HarkonenBade I'm not quite sure about the usefulness of splitting, to me that is mostly a historical thing rather than a practicality. It would make most sense if Rx and Tx were mostly used independently but I really haven't seen that being used anywhere in the wild. In fact I often have to share Rx/Tx together with my interrupt handlers to make use of them (unless I only need one of them).

Maybe it would make sense to actually implement the serial traits directly on the serial port handle to allow direct use of a bi- or unidirectional serial channel with release capability but also keep the possibility to split in case a serial port is meant to be used in two separate places. What do you think?

That sounds reasonable to me, also I'm unsure over the use of generating temporary Tx/Rx instances to impl the Read/Write traits, it would seem more appropriate to move the implementations out to free functions that take a *const USART argument.

Moved it out in separate functions with RegisterBlock, I could remove the USART parameter from Tx & Rx type, not sure if I should.

I'd leave it there, its potentially relevant to distinguish Tx and Rx objects from different USART types.

HarkonenBade · 2019-01-05T22:32:45Z

src/serial.rs

+                    let txpin = ();
+                    let mut serial = Serial { usart, pins: (txpin, rxpin) };
+                    serial.enable(baud_rate, clocks);
+                    serial


As above but for Rx

HarkonenBade

I think there are a few bits that might need thinking about further.

HarkonenBade

I've taken another look over and everything LGTM 👍

therealprof · 2019-01-11T14:16:57Z

src/serial.rs

+    }
+}
+
+/// Tries to write a byte to the uart


Suggested change

/// Tries to write a byte to the uart

/// Tries to write a byte to the UART

therealprof · 2019-01-11T14:17:20Z

src/serial.rs

+    }
+}
+
+/// Tries to read a byte from the uart


Suggested change

/// Tries to read a byte from the uart

/// Tries to read a byte from the UART

therealprof

Would be great if we could get a doc example for unidirectional use. Otherwise LGTM.

HarkonenBade · 2019-01-11T14:28:48Z

Actually, would it also be worth updating at least one of the examples that doesn't really need split under this? Maybe the plain echo?

therealprof · 2019-01-11T14:33:56Z

Actually, would it also be worth updating at least one of the examples that doesn't really need split under this? Maybe the plain echo?

I haven't looked in detail but is this even a possibility, i.e. does the implementation allow for multiple independent uses of the chosen USART to attach multiple independent pins to it? If so that might be construed by some as unsafeness... 😏

Anyway, I'd be okay with modifying the examples or adding another, like a plain hello world, but I strongly favour better documentation over examples.

HarkonenBade · 2019-01-11T14:37:03Z

@therealprof It doesn't let you define multiple usarts with the same pins, but the way the impls of read and write for usarts are worded also implements them for duplex usarts. So a full duplex usart can have both read and write called on it, and thus doesn't require splits if you don't need to send the parts to different places.

therealprof · 2019-01-11T14:39:52Z

@HarkonenBade Sorry, I misunderstood you then. Sure, go ahead.

HarkonenBade · 2019-01-11T14:42:57Z

Though this has raised an interesting question for me, could we shift it so that split() returns (Serial<USART1, TxPin, ()>, Serial<USART1, (), RxPin>) and just eliminate Tx and Rx all together?

It would mean that we would need to be more careful in future if we offer any ability to re-configure the peripheral at runtime.

david-sawatzke · 2019-01-11T14:51:24Z

I think this would make a destructor pretty hard. It also has issues with the listen/unlisten calls (the classic race condition thing). I also don't like two serials of the same port existing, for me it looks like a hack (because it is).

HarkonenBade · 2019-01-11T14:52:18Z

That's fair, it was just a thought based on the pleasing way the read/write impls turned out.

Also add the `Write` implementation for Serial

therealprof

LGTM, thanks!

david-sawatzke force-pushed the serial_mult branch 3 times, most recently from e5ca471 to bd3478f Compare January 5, 2019 22:14

HarkonenBade reviewed Jan 5, 2019

View reviewed changes

src/serial.rs Outdated Show resolved Hide resolved

HarkonenBade reviewed Jan 5, 2019

View reviewed changes

david-sawatzke force-pushed the serial_mult branch from 4edc669 to 8a00351 Compare January 6, 2019 11:42

david-sawatzke added 4 commits January 11, 2019 12:24

Add a tx only and a rx only serial instance

8e0179d

Add changelog entry

ee11a82

Only enable usart tx/rx when needed

2c62bd9

Move read, flush & write in separate functions

ec1fa41

david-sawatzke force-pushed the serial_mult branch from 6d7d30a to ec1fa41 Compare January 11, 2019 11:26

HarkonenBade approved these changes Jan 11, 2019

View reviewed changes

HarkonenBade requested a review from therealprof January 11, 2019 14:12

therealprof reviewed Jan 11, 2019

View reviewed changes

therealprof requested changes Jan 11, 2019

View reviewed changes

Add documentation and update a few examples

c8dd9b1

Also add the `Write` implementation for Serial

therealprof approved these changes Jan 11, 2019

View reviewed changes

therealprof merged commit 0ffbce7 into stm32-rs:master Jan 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a tx only and a rx only serial instance #34

Add a tx only and a rx only serial instance #34

david-sawatzke commented Jan 5, 2019 •

edited

Loading

HarkonenBade commented Jan 5, 2019

HarkonenBade Jan 5, 2019

david-sawatzke Jan 6, 2019

HarkonenBade Jan 6, 2019

david-sawatzke Jan 6, 2019

HarkonenBade Jan 6, 2019

HarkonenBade Jan 6, 2019

therealprof Jan 7, 2019

HarkonenBade Jan 7, 2019

david-sawatzke Jan 7, 2019

HarkonenBade Jan 7, 2019

HarkonenBade Jan 5, 2019

HarkonenBade left a comment

HarkonenBade left a comment

therealprof Jan 11, 2019

therealprof Jan 11, 2019

therealprof left a comment

HarkonenBade commented Jan 11, 2019

therealprof commented Jan 11, 2019

HarkonenBade commented Jan 11, 2019

therealprof commented Jan 11, 2019

HarkonenBade commented Jan 11, 2019 •

edited

Loading

david-sawatzke commented Jan 11, 2019

HarkonenBade commented Jan 11, 2019

therealprof left a comment

	/// Tries to write a byte to the uart
	/// Tries to write a byte to the UART

	/// Tries to read a byte from the uart
	/// Tries to read a byte from the UART

Add a tx only and a rx only serial instance #34

Add a tx only and a rx only serial instance #34

Conversation

david-sawatzke commented Jan 5, 2019 • edited Loading

HarkonenBade commented Jan 5, 2019

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

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

HarkonenBade left a comment

Choose a reason for hiding this comment

HarkonenBade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therealprof left a comment

Choose a reason for hiding this comment

HarkonenBade commented Jan 11, 2019

therealprof commented Jan 11, 2019

HarkonenBade commented Jan 11, 2019

therealprof commented Jan 11, 2019

HarkonenBade commented Jan 11, 2019 • edited Loading

david-sawatzke commented Jan 11, 2019

HarkonenBade commented Jan 11, 2019

therealprof left a comment

Choose a reason for hiding this comment

david-sawatzke commented Jan 5, 2019 •

edited

Loading

HarkonenBade commented Jan 11, 2019 •

edited

Loading