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: remove `initialize` from the UART HIL #1073

Merged
merged 4 commits into from Jul 16, 2018

Conversation

Projects
None yet
6 participants
@ppannuto
Copy link
Member

ppannuto commented Jul 5, 2018

Pull Request Overview

This implements RFC #1035 for the UART HIL. It splits the hardware-specific
initialization (power, clock assignment, pin assignment) from the uart-specific
configuration (baud rate, parity, stop bits, flow control), which is now
controlled by a new configure method for the UART HIL.

Testing Strategy / Help Wanted

  • Testing on hail/imix
  • Testing on nrf family
  • Testing on launchxl
  • Testing on ek-tm4c1294xl

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.
@bradjc
Copy link
Contributor

bradjc left a comment

I think this is a very useful change.

@ppannuto ppannuto force-pushed the uart-hil-no-init branch from a31f221 to 89b31de Jul 6, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 6, 2018

I don't know if anyone has an ek to test on, but I'll test the NRF52 and launchxl today

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 9, 2018

Tested for nrf52dk and launchx, and works. Since I don't think anyone has an ek, I move that can't do better than assume it'll work.

uart: remove `initialize` from the UART HIL
This implements RFC #1035 for the UART HIL. It splits the hardware-specific
initialization (power, clock assignment, pin assignment) from the uart-specific
configuration (baud rate, parity, stop bits, flow control), which is now
controlled by a new `configure` method for the UART HIL.

@bradjc bradjc force-pushed the uart-hil-no-init branch from 9a8b054 to 1f6423b Jul 9, 2018

@bradjc bradjc removed the needs-rebase label Jul 9, 2018

@ppannuto ppannuto dismissed stale reviews from alevy and bradjc via 3e71631 Jul 9, 2018

```

This comment has been minimized.

@niklasad1

niklasad1 Jul 9, 2018

Member

wow, totally missed that we have a changelog now nice!

@niklasad1
Copy link
Member

niklasad1 left a comment

nice, I like these changes!

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 10, 2018

Is it worth revisiting the whole UART HIL?

https://groups.google.com/d/msg/tock-dev/3NtJfzuz-ag/cHxJXWndAwAJ

@bradjc bradjc referenced this pull request Jul 10, 2018

Merged

Kernel: Move `PROCS` array to `Kernel` struct #1111

2 of 4 tasks complete

@ppannuto ppannuto added the last-call label Jul 11, 2018

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 11, 2018

I do think the whole UART HIL should aim to be re-evaluated this release cycle, and I'm working towards that as I can (have family in and out of town this and next week, so off/on for the next few days).

That said, I think this is a step in the right direction, implements something we've discussed for all HILs (#1035), and is blocking a few other PRs right now.

I think the complete UART HIL re-design can build on this change, whose disposition seems positive thus far, so I'm tagging this last-call.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 11, 2018

This is exactly the kind of small change that I've struggled with in my porting work. I strongly disagree with making this last-call.

Two calls ago, we discussed a policy that if we touch a HIL, we should set the bar that we should strive to never touch it again? Basically, I think that we should resolve all the issues raised in #1072 before we start merging pull requests.

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 11, 2018

Part of what I am struggling with in this case is a breadth vs depth problem. #1035 is an effort to remove initialize from every HIL as a kernel-wide policy, that I thought we came to consensus on as a good design principle. I think it should be a goal of this release to address this for each HIL, but an intractably large change to do in one giant shot. Regarding churn, I thought the consensus we came to is that master is permitted to be a bit of a moving target, but releases should have a coherent changeset. To that end, I do think that we should aim to fix the rest of the HILs to remove initialize before the next release, but that is not a reason to block this PR now.

I don't think it's reasonable or feasible to do a breadth and depth of every single HIL in parallel. I don't think we can agree that we should remove initialize from each HIL and expect to get each HIL perfect in the process of removing initialize.

@ppannuto ppannuto referenced this pull request Jul 11, 2018

Open

Tracking: Initialize in HIL interfaces #1112

0 of 6 tasks complete
@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 11, 2018

Towards an adherence strategy for this, I've just created #1112 to track the progress of initialize implementations and created a release-blocker tag for issues that need to be resolved before we can issue the next release. That way we have a mechanism of tracking that releases include a cohesive set of changes.

@ppannuto ppannuto dismissed stale reviews from bradjc, alevy, and niklasad1 via 5ab00cf Jul 13, 2018

Merge branch 'master' into uart-hil-no-init
Conflict with new imix components
@alevy

alevy approved these changes Jul 16, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 16, 2018

This PR is orthogonal to revisiting the UART HIL more broadly. It's purpose is to implement a change across PRs that was discussed in #1035. Because it was a change that was already discussed, and agreed on, a bunch of other changes have been made with the assumption that some version of removing initialize will be merged, and those PRs are now blocking on this.

Given that there are no outstanding issues with this PR other than it prompting a more general look at the UART HIL, we should merge this.

@bradjc

bradjc approved these changes Jul 16, 2018

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 16, 2018

bors r+

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 16, 2018

bors r+

Trying again having enabled bors now :)

bors bot added a commit that referenced this pull request Jul 16, 2018

Merge #1073
1073: uart: remove `initialize` from the UART HIL r=ppannuto a=ppannuto

### Pull Request Overview

This implements RFC #1035 for the UART HIL. It splits the hardware-specific
initialization (power, clock assignment, pin assignment) from the uart-specific
configuration (baud rate, parity, stop bits, flow control), which is now
controlled by a new `configure` method for the UART HIL.

### Testing Strategy / Help Wanted

- [x] Testing on hail/imix
- [x] Testing on nrf family
- [x] Testing on launchxl
- [ ] Testing on ek-tm4c1294xl

### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Pat Pannuto <pat.pannuto@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jul 16, 2018

@bors bors bot merged commit e821691 into master Jul 16, 2018

4 checks passed

bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@bors bors bot deleted the uart-hil-no-init branch Jul 16, 2018

@alevy alevy referenced this pull request Jul 24, 2018

Merged

Use proper pin for CC26xx UART RX #1131

2 of 2 tasks complete

bors bot added a commit that referenced this pull request Jul 24, 2018

Merge #1131
1131: Use proper pin for CC26xx UART RX r=ppannuto a=alevy

### Pull Request Overview

This fixes a regression in the CC26xx UART introduced by a tiny oversight in #1073. The implementation of `configure` was using the stored `tx_pin` twice, for configuring both TX and RX pins. Since RX is configured second, this overrode resulted in the user-chosen TX pin being configured to act as the RX side of the UART, and no TX configured at all.

The result is that the console or debug appeared not to work.

### Testing Strategy

I programmed the launchxl board with a kernel from current `master` as well as the `c_hello` app. Running that results in no output on the UART.

With the fix in this PR, and the same app, output is "Hello World!", as expected.

I ran similar small tests with `panic!` and `debug!`.

### TODO or Help Wanted

_For after this PR_

I'm concerned about the duplication of the PCRM module---the UART module refers to the CC26xx PCRM module, while other drivers refer to the CC26x2 PCRM module. I originally assumed this was the problem until running `git bisect`. I really think we should just get rid of CC26xx for now. But that's for another PR.

### Documentation Updated

- [x] ~~Updated the relevant files in `/docs`, or no updates are required.~~

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Amit Aryeh Levy <amit@amitlevy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment