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

UART HIL Changes #936

Merged
merged 2 commits into from Jun 8, 2018
Merged

UART HIL Changes #936

merged 2 commits into from Jun 8, 2018

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented May 16, 2018

Pull Request Overview

This pull request removes the receive_until_terminator interface from the UART HIL and renames receive_automatic to receive_timeout.

I imagine this one will be a little controversial with @brghena, but it has been two years and receive_until_terminator has never been implemented much less used.

It also renames the trait from UARTAdvanced to UARTReceiveTimeout to be a little more specific.

Testing Strategy

This pull request was tested by compiling the hail board.

TODO or Help Wanted

I think we could do without this PR, but given that the bootloader needs the receive_automatic interface, I think it is a good time to revisit the advanced uart interface.

Documentation Updated

  • Kernel: Updated the relevant files in /docs, or no updates are required.
  • Userland: Added/updated the application README, if needed.

Formatting

  • Ran make formatall.

@bradjc bradjc added the kernel label May 16, 2018
brghena
brghena previously approved these changes May 16, 2018
Copy link
Contributor

@brghena brghena left a comment

Choose a reason for hiding this comment

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

The renaming is good.

As for the removal of receive_until_terminator, I am saddened by its loss. I still believe that it is one of three semantic ways that higher level code interacts with serial streams, and I know that Signpost would be using that functionality if it existed.

However, if the code has never been implemented, and moreover it's unclear how any microcontroller would efficiently implement it, then removing it seems good.

@alevy alevy added the P-Significant This is a substancial change that requires review from all core developers. label May 16, 2018
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

Generally agreed. I think I want to bikeshed the name just a little. UARTReceiveTimeout sounds like a receive with a timeout, but that's not quite what this interface is. Critically, the interface will not timeout until at least one byte is received. Perhaps something like UARTReceieveTemporallyFramed or UARTReceiveTimedFrame or UARTRecieveMessageSeparatedByTime

niklasad1
niklasad1 previously approved these changes May 17, 2018
@alevy
Copy link
Member

alevy commented May 17, 2018

anybody implementing the UART HIL should be aware of this. That at least includes @cpluss, @phil-levis and @george-hopkins, though I believe they don't use the affected traits

@cpluss
Copy link
Contributor

cpluss commented May 17, 2018

cc26xx does not use the advanced traits at the moment :)

alevy
alevy previously approved these changes May 17, 2018
Copy link
Contributor

@phil-levis phil-levis left a comment

Choose a reason for hiding this comment

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

I'd agree with Pat -- if the semantics is "timeout after a byte is received", this is not "receive_timeout", and I know I would for sure be totally confused and surprised by this behavior. Is there a better name, or should the implementation actually implement what the interface implies. This seems like a very SAM4L abstraction that's being unduly generalized?

@bradjc
Copy link
Contributor Author

bradjc commented May 21, 2018

UARTReceiveTimeout sounds like a receive with a timeout, but that's not quite what this interface is. Critically, the interface will not timeout until at least one byte is received.

if the semantics is "timeout after a byte is received", this is not "receive_timeout"

Should we just leave the name receive_automatic? And I actually think receive_timeout is fine, since if the system times out before anything is received then why is it called receive timeout?

This seems like a very SAM4L abstraction that's being unduly generalized?

We need something for the bootloader. The other interface we could use is a callback-per-byte interface, which we probably don't want. With the bootloader we currently have two uses of this interface at the capsule level (the other being the nrf_serialization), which is pretty good for a tock hil interface.

@ppannuto
Copy link
Member

Yeah, I think receive_automatic sounds fine.

As for the language implied by "receive_timeout", the semantics I'm used to (e.g. Python or Arduino) is to receive until a timeout, where it's perfectly valid to receive 0 bytes when the timeout occurs. I don't know of any other interface that works quite like our receive automatic -- though I think it's useful. I don't think it's too SAM4L specific, really it's more an anachronism of the protocol used by the chips talking to the SAM4L, which makes it a generally useful interface I think.

@bradjc
Copy link
Contributor Author

bradjc commented May 21, 2018

Ok I left the trait renamed, but the function is back to what it used to be called.

brghena
brghena previously approved these changes May 25, 2018
@bradjc
Copy link
Contributor Author

bradjc commented May 25, 2018

Ok rebased, and changed the name again:

trait: UartReceiveAdvanced
fn: receive_automatic()

This makes the trait more specific about what it is actually doing, and avoids any issues with what is timing out or when.

alevy
alevy previously approved these changes May 25, 2018
The method isn't used and isn't implemented, and hardware doesn't seem
to support it.
It is now UARTReceiveTimeout and contains one function which times once
bytes are no longer being received.
@bradjc
Copy link
Contributor Author

bradjc commented May 25, 2018

Rebased. @phil-levis

@alevy
Copy link
Member

alevy commented Jun 5, 2018

@phil-levis do you have more comments after @bradjc addressed your or can we merge?

Copy link
Contributor

@phil-levis phil-levis left a comment

Choose a reason for hiding this comment

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

Looks good.

@phil-levis
Copy link
Contributor

Sorry for the delay -- retreats and stuff. Unblocked it.

@alevy alevy merged commit 817b56b into master Jun 8, 2018
@alevy alevy deleted the uart-changes branch June 8, 2018 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel P-Significant This is a substancial change that requires review from all core developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants