Skip to content

drivers/periph/uart: remodeled UART interface#4114

Merged
haukepetersen merged 43 commits intoRIOT-OS:masterfrom
haukepetersen:opt_periph_uart
Oct 28, 2015
Merged

drivers/periph/uart: remodeled UART interface#4114
haukepetersen merged 43 commits intoRIOT-OS:masterfrom
haukepetersen:opt_periph_uart

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Oct 20, 2015

supersedes #3265

After many iterations #3265 became un-managable, so I decided for a clean approach using the latest consensus. The UART interface is now significantly simplified, only one write function remains. The concepts summarized:

  • receiving is done purely ISR based
  • transmitting is handled internally by the low-level driver implementation, e.g. blocking, isr-based, DMA-based

All existing CPUs are adapted to the interface changes -> dropping ~1500 lines of code :-)

I did however just adapt the UART drivers for the introduced interface changes, I did not optimize any of the drivers -> this can/has to be done CPU by CPU in follow up PRs, as this is also easier to review...

Tested for:

  • airfy-beacon
  • arduino-due
  • arduino-mega2560
  • avsextreme
  • cc2538dk
  • f4vi1
  • fox
  • frdm-k64f, only checked UART_0
  • iot-lab_M3, only checked UART_0
  • msb430
  • mbed_lpc1768, only checked UART_0
  • msba2
  • msbiot
  • mulle UART_0 and UART_1
  • nucleo-f091
  • nucleo-f303
  • nucleo-f334
  • nucleo-l1
  • nrf51dongle
  • openmote
  • pba-d-01-kw2x, only checked UART_0
  • pca10000
  • pca10005
  • pttu
  • saml21-xpro
  • samr21-xpro
  • spark-core
  • stm32f0discovery
  • stm32f3discovery
  • stm32f4discovery
  • telosb
  • udoo
  • wsn430
  • yunjia-nrf51822
  • z1

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Oct 20, 2015
@haukepetersen haukepetersen added this to the Release NEXT MAJOR milestone Oct 20, 2015
This was referenced Oct 20, 2015
Closed
Merged
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 20, 2015

#4115 gives an example, how the changed interface can be utilized for improved UART driver implementations...

This was referenced Oct 22, 2015
Closed
Merged
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 23, 2015

@gebart, @jfischer-phytec-iot, @kaspar030: are you guys ok with these changes in the UART interface?

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 23, 2015

rebased

@thomaseichinger
Copy link
Copy Markdown
Member

thomaseichinger commented Oct 23, 2015

After discussions in #3265, do we consent on this approach? Or are their any objections? @daniel-k @gebart @kaspar030 @jfischer-phytec-iot @OlegHahm @sgso @PeterKietzmann

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Oct 23, 2015

I like the simplified API. You have my approval.

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 23, 2015

I also like the approach on the first sight. I just started to look at it again and try it with some boards

@thomaseichinger
Copy link
Copy Markdown
Member

thomaseichinger commented Oct 23, 2015

@PeterKietzmann Great, could you note down the boards you tested here?

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 23, 2015

Of course...

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 23, 2015

I placed a list of boards in the PR description above and checked some boards that I currently have available (marked them there). Until now I didn't face any problems

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 23, 2015

damn, I actually thought we could go around testing every board, as this takes forever again... I mean this PR doesn't change anything logical in the uart drivers, so a good code review and testing of some selected boards should suffice I was hoping...

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Oct 23, 2015

Since this does not change the logic (much), it should be enough to do a quick manual code review, and some spot checks by running tests/periph_uart on various boards along with testing the users on any platform (stdio, gnrc_slip, drivers/xbee).
I'll try out slip on mulle/k60 some time during the weekend.

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 26, 2015

I don't want to be responsible for this PR, so I leave the decision up to you ;-) ! I agree with your arguments, even if testing seems to be the "safer" way in general. With my above stated list I did STDIO checks for different hardware. @gebart wants to test slip. So the last thing to test will be drivers/xbee. Any volunteers? I don't have the hardware available. Is there anything else you want me to do @haukepetersen, @gebart ?

This was referenced Oct 27, 2015
Closed
Closed
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 27, 2015

@PeterKietzmann: yes, give an ack :-)

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 27, 2015

ACK for the part that I tested (described above).

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 27, 2015

thx!

@gebart: did you get to test this on the mulle board?

@haukepetersen
cpu/samd21: adapted UART driver
a93f15b
@haukepetersen
cpu/saml21: adapted UART driver
6567366
@haukepetersen
cpu/stm32f0: adapted UART driver
b71d8ea
@haukepetersen
cpu/stm32f1: adapted UART driver
59b935b
@haukepetersen
cpu/stm32f3: adapted UART driver
9741267
@haukepetersen
cpu/stm32f4: adapted UART driver
1eb63f2
@haukepetersen
cpu/stm32l1: adapted UART driver
19cd4b3
@haukepetersen
boards/msb-430: adjusted to UART changes
a8502e2
@haukepetersen
boards/msb-430h: adjusted to UART changes
a4372ce
@haukepetersen
boards/msba2: adjusted to UART changes
eb46b6b
@haukepetersen
boards/pttu: adjusted to UART changes
bfdc3e2
@haukepetersen
boards/telosb: adjusted to UART changes
c62f39a
@haukepetersen
boards/wsn430-common: adjusted to UART changes
0e1e16d
@haukepetersen
boards/wsn430-v1_3b: adjusted to UART changes
fe7e2d1
@haukepetersen
boards/wsn430-v1_4: adjusted to UART changes
49efe3f
@haukepetersen
boards/z1: adjusted to UART changes
ddb8765
@haukepetersen
drivers/xbee: adapted to UART interface changes
82afb07
@haukepetersen
net/slip: adjusted to UART interface changes
19bb76f
@haukepetersen haukepetersen mentioned this pull request Oct 27, 2015
Merged
@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 27, 2015

Travis disagrees in some points :-(

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 27, 2015

I see, will fix tomorrow.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 28, 2015

fixed those Travis issues: by added FEATURES_PROVIDED += periph_uart to some msp430 boards, some tests/examples were now build for these boards while not fitting memory wise...

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 28, 2015

My ACK holds for the parts that I tested. But I don't dare to hit the button as this doesn't cover the whole PR. Anyone else?

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 28, 2015

yes, with pleasure :-)

haukepetersen added a commit that referenced this pull request Oct 28, 2015
@haukepetersen
Merge pull request #4114 from haukepetersen/opt_periph_uart
85e05d4 
drivers/periph/uart: remodeled UART interface
@haukepetersen haukepetersen merged commit 85e05d4 into RIOT-OS:master Oct 28, 2015
@haukepetersen haukepetersen deleted the opt_periph_uart branch October 28, 2015 09:50
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Oct 28, 2015

Nice one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@thomaseichinger thomaseichinger

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2015.12

Development

Successfully merging this pull request may close these issues.

7 participants

@haukepetersen @thomaseichinger @jnohlgard @PeterKietzmann @jfischer-no @daniel-k @kaspar030