ROBOTIS-GIT / OpenCR

Software for ROS Embedded board (a.k.a. OpenCR). OpenCR means Open-source Control Module for ROS.
Apache License 2.0
386 stars 238 forks source link

Non blocking Serial = More limited in some of the lower level changes. #132

Closed KurtE closed 6 years ago

KurtE commented 6 years ago

Add support for allowing the SerialX objects to do writes without blocking.

This one is using a different approach than the PR I have for OpenCM(https://github.com/ROBOTIS-GIT/OpenCM9.04/pull/38) where I changed the bottom level driver to add function support to try to preload the UARTs output register, and then only setup to use fifo queue and interrupts...

This version uses the current functions: HAL_UART_Transmit_DMA and HAL_UART_Transmit_IT to do the output code. It keeps a FIFO queue as part of the high level UARTClass object, which it calls either the DMA or IT depending on how the Serial port is configured.

Issue with DMA. Both Serial3 and SPI(SPI2) both want the same the same DMA1_StreamX, so put code in place that both of these call an API to remember the DMA object, and this function has it's own copy of DMA1_StreamX ISR functions which calls the underlying DMA function with the appropriate handle. In these cases the Stream3(DXL) TX code falls back to using ISR output.

Tested DXL code at up to 4MBS.

Tried with both Serial2 and Serial3 functions. I also have version of dxl_monitor program that added a SSD1306 (Teensyview) running on SPI2, code showed that DXL fell back to using IT function and still ran at 4mbs.

More information up on the Issue: https://github.com/ROBOTIS-GIT/OpenCR/issues/131

Again as I mentioned in the other PR. Would suggest a full code review. Also more testing. I will be trying out some additional programs like maybe my Turtlebot, plus maybe Hexapod code.

Kurt

KurtE commented 6 years ago

As I mentioned in #131 yesterday. The use of non blocking writes can help expose issues with Serial ports using ISR method to receive data, especially if other ISRs are active...

To help combat this, I put the stuff in place to allow all of the SerialX objects to use DMA. I added code for DMA2 to functions to allow other callers to use DMA streams. Also with this hopefully there is some fallback code (probably needs more testing), where for example both Serial2(USART2_TX) and Serial4(UART8_RX) want the same DMA stream (DMA1_Stream6). Hopefully if both Serial objects are used the RX one should be the one that wins...

But looking promising, From test program over on issue, with DMA on all of them:

Serial ping pong test
Line in: abcdefghijklmnopqrstuvwzyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ
Echoed : abcdefghijklmnopqrstuvwzyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ
Delta Time 6325us, Cnts: 62 62 62 62 time per char: 102 psudo baud 1612

Note the calculation for psuedo baud is wrong: The calculations should probably be something like; (1000000/(6325/6210))10 = 98024 which is not bad as compared to 115200 screenshot

Also from issue, without the non blocking writes same test took about 17458us to output 63 characters, so the psuedo baud would be about: 36087

Assuming if going to DMA for RXs is ok with you, still more things to fix: a) Remove the 2K RX ring buffer from USART class and support code b) Fix flush add flushRx c) Fix Serial.read() if queue is empty d) Fix Serial.peek

Most of these should be done any way.

Update

I think I have fixed b) c) and d) in last commit. Also had commit to add keywords.txt.

Probably done for now until I hear something

OpusK commented 6 years ago

Hi, @KurtE

I am always thankful for your contribution. Just as I left it in your OpenCM PR, I think we need to worry about changing the DYNAMIXEL sdk code.

@kijongGil, please review.

KurtE commented 6 years ago

@OpusK and reviewers.

As I believe I mentioned, in here and or issue... I am guessing that for the underlying changes you will prefer some of this over the method I did for OpenCM. In particular in OpenCM, there was new custom code that, that tries to force feed the UART if possible and if not then setup ISR call back function that feeds the UART... Which has the benefit of maybe a little less overhead for first couple of characters...

This version uses the current ISR and DMA write functions. When you start a write it saves away as much as it can in the buffer (min of number of bytes passed in and free space in buffer). It starts up the request and returns. If while that one is being processed, it continues to feed data into the queue, until the first transfer completes, at which time, the code starts up the ISR or DMA

If you are worried about making changes to DynamixelSDK. Can simplify. Don't have to use the Serial functionality I added that can automatically control the direction pin.

Could probably get away with just adding a call to DYNAMIXEL_SERIAL.flush(); This could either go directly in: PortHandlerArduino::writePort and/or in: void PortHandlerArduino::setTxDisable()

This would semi disable some of the gains of non-blocking writes, but for those portions of code that do TXRX calls, this would already happen. So this would only defeat in cases which are TX only. But still will allow other things to output without holding things up.

Note: I have not committed this yet, but I have code in place that you can configure the system to use ONLY DMA for Serial input.... Doing so allows you to remove the 2K buffer from each UARTSerial object which saves over 8K of memory.

KurtE commented 6 years ago

@OpusK and others,

I made a version of these changes where I removed several of the changes. Like having the SerialX object control the setting of TX or RX. I removed the definition of the DXL direction pin as being an Arduino pin.

The changes to Dynamixel SDK limited to calling flush whenever we switch to RX mode before we change the state of the direction pin.

This version has the option I mentioned above to do all UARTS reads using DMA...
Can issue as separate Pull Request if that would help?

Forgot to mention that I did push it up to my fork: https://github.com/KurtE/OpenCR/tree/Non_blocking_Serial_more_minimal

OpusK commented 6 years ago

@KurtE,

I checked the "Non_blocking_Serial_more_minimal" branch you mentioned above. I do not see any problems. Please PR above branch.

I think that if we handle the Flush function on other platforms well, the code you originally PR does not seem to matter. However, I do not have the authority to make this decision, so I will hand it over to the SDK representative.

So, we would like to work on your new PR basis :)

KurtE commented 6 years ago

We should probably close this one out as the other PR took care of this functionality.

OK to Close?

OpusK commented 6 years ago

I think I'd better close it. Thank you.