Skip to content

nrf: implement busio.UART #1039

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

Closed
dhalbert opened this issue Jul 17, 2018 · 43 comments
Closed

nrf: implement busio.UART #1039

dhalbert opened this issue Jul 17, 2018 · 43 comments
Assignees
Labels

Comments

@dhalbert
Copy link
Collaborator

No description provided.

@dhalbert dhalbert added this to the 4.0 Beta milestone Jul 17, 2018
@hathach hathach self-assigned this Sep 18, 2018
@hathach
Copy link
Member

hathach commented Sep 18, 2018

While working on uart, @arturo182 is it ok now if I remove the macro that use the jlink cdc instead of native cdc. That option is there for you only :)

@arturo182
Copy link
Collaborator

Which cdc do you mean?

@hathach
Copy link
Member

hathach commented Sep 18, 2018

the jlink cdc that uses the hwuart on pca10056 CFG_HWUART_FOR_SERIAL
https://github.com/adafruit/circuitpython/blob/master/ports/nrf/supervisor/serial.c#L35

@arturo182
Copy link
Collaborator

Yes, it's fine to remove that :)

@hathach
Copy link
Member

hathach commented Sep 18, 2018

anyone knows this function got called, or it is called by system board_uart
https://github.com/adafruit/circuitpython/blob/master/ports/nrf/board_busses.c#L108

@hathach
Copy link
Member

hathach commented Sep 18, 2018

ah ha, I figured it out, it is the default constructor, sort of :D It is used for default UART object, but not included yet by nrf port.

@hathach
Copy link
Member

hathach commented Sep 19, 2018

@dhalbert the default uart for pca10056 should be P0.8 and P0.6 instead of P1.1 & P1.2. It is connected to jlink cdc allow for quick testing. Furthermore if jlink cdc is not connected, those pins are in tri-state, which is safe to use with other device. Please let me know if I missed something.
https://github.com/adafruit/circuitpython/blob/master/ports/nrf/boards/pca10056/mpconfigboard.h#L49

@hathach
Copy link
Member

hathach commented Sep 19, 2018

Ah, I see, seem like you want to keep the pin compatible with Arduino layout :D . Sorry, didn't realize that until I check the pcb

@dhalbert
Copy link
Collaborator Author

@hathach right, exactly.

@hathach
Copy link
Member

hathach commented Sep 19, 2018

@dhalbert @tannewt should the self->buffer be freed in common_hal_busio_uart_deinit() , or we can just leave it to gc

https://github.com/adafruit/circuitpython/blob/master/ports/atmel-samd/common-hal/busio/UART.c#L217

@dhalbert
Copy link
Collaborator Author

Leave it to gc. It is allocated by gc_alloc() as a regular garbage-collectable object.

@hathach
Copy link
Member

hathach commented Sep 19, 2018

ah thanks, this new to me, I will just skip the free for nrf52 as well.

@hathach
Copy link
Member

hathach commented Sep 20, 2018

probably the same hw issue that I encountering. The first 2 irq uart request will fail miserably without real reason. UART is not too easy after all ~ . ~
https://devzone.nordicsemi.com/f/nordic-q-a/33677/nrf52840-pin-uart-rx-bug

@dhalbert
Copy link
Collaborator Author

@hathach that devzone post is very confusing. I wonder if that's the PDK board.

@hathach
Copy link
Member

hathach commented Sep 20, 2018

@dhalbert that is what I am seeing here as well with my board. At least I know why it doesn't work out the way I think. Using bare-metal is much easier for busio in comparison with the nrfx_uart. Though, I am getting it done by just ignoring first 2 irq error

@hathach
Copy link
Member

hathach commented Sep 20, 2018

@dhalbert If you have time, I could push the code for you test there before I clean them up.

@dhalbert
Copy link
Collaborator Author

i could try it with a GPS

@hathach
Copy link
Member

hathach commented Sep 20, 2018

You don't need extra hw to test since only the write() work now. The bug prevent me to do subsequent receiving since it failed at the beginning.

here is the code
https://github.com/hathach/circuitpython/blob/nrf52_uart_io/ports/nrf/common-hal/busio/UART.c#L75
board.UART() will call the first nrfx_uart_rx() which will trigger error 4 (frame error) ---> nrfx will cancel transfer internally.

Just run u.read() : this will printf the previous error and call nrfx_uart_rx() again.
Then call u.read() again to get the previous error of the 1st read which is 0x08 break error.
afterward no more error, call more will result in NRFX_BUSY code.

To test with u.write() you can hook up FTDI cable, it should work.

Adafruit CircuitPython 4.0.0-alpha-1079-g9c2530687-dirty on 2018-09-21; PCA10050
>>> import board                                                                
>>> u = board.UART()                                                            
>>> u.read()                                                                    
error = 0x00000004                                                              
rx count = 0x00000000                                                           
b''                                                                             
>>> u.read()                                                                    
error = 0x00000008                                                              
rx count = 0x00000000                                                           
b''                                                                             
>>> 
>>> u.read()                                                                    
rx count = 0x00000000                                                           
b''                                                                             
>>> u.read()                                                                    
rx count = 0x00000000                                                           
Traceback (most recent call last):                                              
  File "<stdin>", line 1, in <module>                                           
AssertionError: error = 0x0BAD000B   

@dhalbert
Copy link
Collaborator Author

Are you using UART or UARTE?

@dhalbert
Copy link
Collaborator Author

I asked for more info in that DevZone case.

@hathach
Copy link
Member

hathach commented Sep 20, 2018

UART since the rest of repo use it. UARTE is just having DMA to move data into buffer, not really needed for cpy I guess. Also the DMA demands data must be on the SRAM, flash address (i.e global const [] ) won't work. I am not sure if cpy use any of those.

@dhalbert
Copy link
Collaborator Author

What exactly is the error you are getting? Is this relevant? https://devzone.nordicsemi.com/f/nordic-q-a/37521/nrf52840-usb_cdc_acm-uart/145013#145013

I saw one DevZone case that recommended using UARTE over UART in nrf52832, but now I can't find it again.

@hathach
Copy link
Member

hathach commented Sep 20, 2018

error 4 (frame ) then error 8 (break). oh way, I think I found my own error lol, let's me test a bit more.

@hathach
Copy link
Member

hathach commented Sep 20, 2018

facepalm, I just assign the txd 2 time :( . That is high price to pay here . . Working late is not good at all lol
https://github.com/hathach/circuitpython/blob/nrf52_uart_io/ports/nrf/common-hal/busio/UART.c#L108

Sorry for your time with my stupid mistake @dhalbert

@dhalbert
Copy link
Collaborator Author

no problem, but time to sleep!

@hathach
Copy link
Member

hathach commented Sep 20, 2018

Do you prefer UARTE to UART, I could upgrade it to UARTE. Though, you must make sure all data is in SRAM not flash, and we must malloc buffer if needed. I think it is not really gain much benefit, since the uart is slow, cpu time for moving data is not much at all.

@dhalbert
Copy link
Collaborator Author

No, I don't prefer, just thought that UARTE might not have the problem, and now it's moot.

@hathach
Copy link
Member

hathach commented Sep 20, 2018

uart io should be done, I will hook an GPS wing to test tomorrow. Phew, too much time for the easy uart. Hopefully there is no more bugs :D

@dhalbert
Copy link
Collaborator Author

I'm rethinking UARTE vs UART: Advantage of UARTE is that there are two instances on '840, so we can have more than one UART channel. I understand the issue of of EasyDMA not working from flash, though we can copy to RAM.

Also UART is deprecated, though I don't understand how significant that is. If there were a 58x0 after the 5840, maybe they'd drop UART?

@hathach
Copy link
Member

hathach commented Sep 21, 2018

No problems, I will make the switch. the flow is similar to what I am doing anyway. IRQ vs DMA only has diff that cpu must move the data manually. But I think I will test what I have first before switching. I will also update the serial.c as well. currently serial.c use polling method, so there should be no impact at all.

@tannewt
Copy link
Member

tannewt commented Sep 21, 2018 via email

@hathach
Copy link
Member

hathach commented Sep 21, 2018

@tannewt yeah, I am moving it to uarte now.

@tannewt
Copy link
Member

tannewt commented Sep 21, 2018 via email

@hathach hathach mentioned this issue Sep 25, 2018
@dhalbert
Copy link
Collaborator Author

dhalbert commented Oct 5, 2018

Still to implement:

  • Using both instances on nRF52840.
  • Add uart_init() to disable on soft restart.
  • Change _VERIFY_ERROR to be more user-friendly.

@hathach I will take these up later.

@tannewt
Copy link
Member

tannewt commented Jan 11, 2019

This has been done right?

@ladyada
Copy link
Member

ladyada commented Jan 11, 2019

its been solid so far, @jerryneedell has done the most testing

@jerryneedell
Copy link
Collaborator

It’s been working great on the particle boards

@dhalbert
Copy link
Collaborator Author

It only provides one UARTE, and there are two available. Not sure the uart_init() has been fixed either. See #1039 (comment) above.

@dhalbert dhalbert reopened this Jan 11, 2019
@dhalbert
Copy link
Collaborator Author

Nope, no uart_init() yet.

@jerryneedell
Copy link
Collaborator

oops - sorry about the premature close...

@tannewt
Copy link
Member

tannewt commented Feb 14, 2019

@dhalbert Can we close this now after #1541 ?

@dhalbert
Copy link
Collaborator Author

Yes, I thought it was going to get closed automatically.

@tannewt
Copy link
Member

tannewt commented Feb 14, 2019

It's not super smart about how when it knows to close: https://help.github.com/articles/closing-issues-using-keywords/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants