Uarts - non blocking writes? about opencm9.04 HOT 16 CLOSED

@OpusK

I am having fun, which is the main point. Hopefully making some reasonable contributions along the way :)

As for ROScon23018, nope, I am simply a retired Software Engineer who still likes to dabble as a hobbyist.
The XEL stuff as well as some of the other projects mentioned up on: https://discourse.ros.org/t/introducing-the-xel-network-modular-h-w-ecosystem-over-ros2/6050
Looked interesting. So maybe some more things to learn about...

from opencm9.04.

from opencm9.04.

Starting to look at my a) above.
Here is a look at some output from my private logical version of firmware for OpenCM... Where I try to intercept any packets that go to device 200. So this one buffers up the bytes until it knows it is not for my logical device and then outputs them. gets the length and outputs it and then gets the rest of the parameters, and output them one at a time. And from this you can see how much time is lost between bytes due to going through the layers and allowing the output buffer to go empty (waiting for TC for each output).

With current state of code, probably will change my firmware code to buffer the whole packet up and output as one write.

In case anyone is interested in these code changes, they are up at:
https://github.com/KurtE/OpenCM9.04/tree/uart_write_changes
Could issue PR if wanted (now includes update lib_f103.a file)

from opencm9.04.

Hi @OpusK

Again not sure if best to discuss here or on RobotSource or ...

As I mentioned above doing a) can help out speed wise IF you organize your code to buffer up stuff and do buffer outputs. However that type of code can slightly slow things down IF you do b). That is you lose a little the system could be doing the serial outputs while you are setting up your buffer...

Note: I don't see where HAL_UART_Transmit_IT or HAL_UART_Transmit_DMA gain you much here as they are still both geared at a single write buffer at a time... Maybe I am missing something? Also with DMA not sure worth it as limited number of channels...

So I do see multiple ways of achieving some of this.

I see a couple of other options:

d) (Don't want to confuse with original a. ) - Could create a version of:

HAL_StatusTypeDef HAL_UART_Transmit(UART_HandleTypeDef *huart, uint8_t *pData, uint16_t Size, uint32_t Timeout)

Which does not wait for TC (removes the code)

    if(UART_WaitOnFlagUntilTimeout(huart, UART_FLAG_TC, RESET, Timeout) != HAL_OK)
    { 
      return HAL_TIMEOUT;
    }

Or makes it conditional on parameter passed in...

Would either need to add call to wait on TC (example: Serial2.flush() before changing direction
May also want to again change code above it in DynamixelSDK which set/clear the IO pin associated with half duplex support.

e) Probably better solution is to change the write code to be more like Teensy or Arduino Due/M0 like code... Example from Arduino/SAM code:

size_t UARTClass::write( const uint8_t uc_data )
{
  // Is the hardware currently busy?
  if (((_pUart->UART_SR & UART_SR_TXRDY) != UART_SR_TXRDY) |
      (_tx_buffer->_iTail != _tx_buffer->_iHead))
  {
    // If busy we buffer
    int nextWrite = (_tx_buffer->_iHead + 1) % SERIAL_BUFFER_SIZE;
    while (_tx_buffer->_iTail == nextWrite)
      ; // Spin locks if we're about to overwrite the buffer. This continues once the data is sent

    _tx_buffer->_aucBuffer[_tx_buffer->_iHead] = uc_data;
    _tx_buffer->_iHead = nextWrite;
    // Make sure TX interrupt is enabled
    _pUart->UART_IER = UART_IER_TXRDY;
  }
  else 
  {
     // Bypass buffering and send character directly
     _pUart->UART_THR = uc_data;
  }
  return 1;
}

void UARTClass::IrqHandler( void )
{
  uint32_t status = _pUart->UART_SR;

  // Did we receive data?
  if ((status & UART_SR_RXRDY) == UART_SR_RXRDY)
    _rx_buffer->store_char(_pUart->UART_RHR);

  // Do we need to keep sending data?
  if ((status & UART_SR_TXRDY) == UART_SR_TXRDY) 
  {
    if (_tx_buffer->_iTail != _tx_buffer->_iHead) {
      _pUart->UART_THR = _tx_buffer->_aucBuffer[_tx_buffer->_iTail];
      _tx_buffer->_iTail = (unsigned int)(_tx_buffer->_iTail + 1) % SERIAL_BUFFER_SIZE;
    }
    else
    {
      // Mask off transmit interrupt so we don't get it anymore
      _pUart->UART_IDR = UART_IDR_TXRDY;
    }
  }

  // Acknowledge errors
  if ((status & UART_SR_OVRE) == UART_SR_OVRE || (status & UART_SR_FRAME) == UART_SR_FRAME)
  {
    // TODO: error reporting outside ISR
    _pUart->UART_CR |= UART_CR_RSTSTA;
  }
}

Obviously would need to change to use the right variable and register names...

Not sure if using their RingBuffer class helps at all? I see that you have this as part of CORE, but really does not add any useful thing? Other than standardized (128 byte queue), with two standard variables, iHead and iTail...

What is good about some of these implementations, is that unlike the HAL_UART_Transmit_IT function, which always relies on using an interrupt to output even the first byte, it sees the queue was empty and the TX empty flag is set and directly outputs the byte at that time... If I were doing this, later may also set the TC interrupt if I wanted to enable Half duplex IO pin support. Regardless would implement the flush() function to wait until queue is empty and TC is set...

I would think there already are versions of Arduino IDE for STM32 based boards which already have buffered writes... I think I maybe have found one:
https://www.stm32duino.com/viewtopic.php?t=3626
Which points to: https://github.com/rogerclarkmelbourne/Arduino_STM32

But assuming I get this to work, is this something you would want?

EDIT I played around some today with making an Async Write function. I have it setup currently only on Serial2, where you can optionally specify a logical Ring buffer structure which then converts the write functions to be Asynch.

Current WIP is in another new branch: https://github.com/KurtE/OpenCM9.04/tree/uart_write_change_async

from opencm9.04.

Hi, @KurtE

I'm sorry for too late reply.

Current WIP is in another new branch: https://github.com/KurtE/OpenCM9.04/tree/uart_write_change_async

awesome! I am always thankful for your enthusiastic contribution.
As you said, in OpenCM9.04 there was no buffering capability and there was much room for improvement.

But assuming I get this to work, is this something you would want?

Being open source, we welcome these contributions.
However, the key point is whether changes in these major APIs will affect existing examples or applications.

By the way, did you add or modify the HAL Driver?
Or is it the latest version of the HAL Driver?
We are avoiding arbitrarily modifying the HAL Driver if possible. If you need low-level access, we recommend using the HAL LL Driver or direct registers.

from opencm9.04.

Hi @OpusK,

Did not have much time today to play around. Was playing around with my emulate your firmware sketch and trying to fix a few things... Currently trying to figure out why my code is crashing when R+ Manager asks it for 374 registers at once... Probably a simple overwrite of something...

I am modifying the HAL layer to add a few new methods to do the FIFO support, I defined a struct version of the RingBuffer as some code that includes some of the headers are C and not C++ and besides the ARduino class only had two method, the constructor (sets variables to 0) and one set function, which Arduino does not use in their write fifo code anyway.

I could past in the changes, here but they are in the github link...

Am also playing around with adding in the functions to have the Async write code be able to set the Transmit Pin state to HIGH when you do a write and to LOW when we process the TC interrupt which is only activated with the queue is empty. Once I have that in place will update some of the functions called by DynamixelSDK and Workbench, that when you call either rxPacket it will first do a flush() to the Serial object to make sure the previous write has completed before starting it's timeout...

Will see about where some of these changes should go. A lot of their stuff is pretty primitive in these layers.

I wonder at times, if it might be worthwhile to try converging toward some other STM32 Arduino installs?
Like;
https://github.com/GrumpyOldPizza/arduino-STM32L4 (Used on STM32L4 boards)
Or https://github.com/stm32duino

But right now taking small steps!

from opencm9.04.

Hi @OpusK,

I checked in another update in my new branch: https://github.com/KurtE/OpenCM9.04/tree/uart_write_change_async

I believe it is working a lot better now. I first fixed a few issues I had with just enabling it on Serial2. Tested with simple app:

void setup() {
  // put your setup code here, to run once:
  while (!Serial && millis() < 5000) ;
  Serial.begin(115200);
  Serial2.begin(115200);
  pinMode(10, OUTPUT);

  pinMode(0, OUTPUT);
  pinMode(1, OUTPUT);
  pinMode(2, OUTPUT);
  pinMode(3, OUTPUT);
}

int loop_count = 0;
void loop() {
  Serial.printf("Loop Count %d\n", ++loop_count);

  digitalWriteFast(10, HIGH);
  Serial2.println("This is a test to see if everything is working");
  digitalWriteFast(10, LOW);

  digitalWriteFast(10, HIGH);
  Serial2.println("Next line of text");
  digitalWriteFast(10, LOW);
  delay(5);
  for (uint8_t i = 'A'; i <= 'Z'; i++) {
    digitalWriteFast(10, HIGH);
    Serial2.write(i);
    digitalWriteFast(10, LOW);
  }
  digitalWriteFast(10, HIGH);
  Serial2.flush();
  digitalWriteFast(10, LOW);

  delay(250);
}

Then enabled it on Serial1 and Serial3 as well. I changed the default Serial read buffer sizes from 256 down to 128 and created the ring buffer for writes with 128 bytes as well. As to not add a lot to memory usage.

I am now testing with my emulating OpenCM firmware... I also have the Test program use new Serial API to have it automatically handle the write enable pin for the serial port. Current WIP code for that app is up in my Open_CM904 project.
Here is a Saleae logic analyzer screen shot showing some debugging stuff where I would echo everythign I receive from USB to USB2, while when it is determined that the bytes would not be processed internally it echos those to in this case Serial1 (line 0) When this debug is going on, you can see splits where I would call the Serial3.write() with the portions of message. First part is the part I gathered until I knew it was not to be processed internally...

When I am not doing this main debug output to Serial2, the whole Serial3 output at 1mbs shows no gaps... The bright side is before this asynchronous output support, the outputs to Serial2, would output completely before anything showed up on Serial3...

Again a lot of this appears to be working great. Need to work more on the DynamixelSDK and workbench to make sure they use it.

Also need to debug a hang when the R+ Manager asks for 374 registers in one message. It errors out and then finally hangs... Need to see what is going on in USB. May also write simple test to see if simply writing a large number of bytes like 400 to USB crashes system. And then verify it happens as well with released Arduino code.

EDIT: Fixed my sketch bug for reading 374 registers...

from opencm9.04.

Quick update: I did a Pull Request #38

So you can hopefully take a look and let me know what you think and/or you see things you would like updated.

from opencm9.04.

@KurtE

Sorry for late reply you.
Your work is so cool!
Thank you so much for your contribution.
I will comment on your PR after review.

from opencm9.04.

@OpusK,
Thanks, I know you are busy with lots of other stuff like XEL (which I want to play with) And maybe see if it makes sense to use with other hardware... Other topic)

Also currently stated to play with OpenCR to add similar support :D Will put more comments on PR

from opencm9.04.

@KurtE,

I am always thankful for your contribution :)

And, thank you for your interest in XEL Network :) Do you participate in this ROScon2018?

from opencm9.04.

@KurtE,

I was impressed by your attitude :)
And, I also want you to be free to join XEL Network project. :)

Thanks!

from opencm9.04.

@OpusK, @robotpilot

Wondering the state of this?

If waiting on Dynamixel SDK - I believe it is the same as stuff in OpenCR now.

My guess is since other PR's have happened since this, that the lib_f103.a file in this is out of date. Not sure if I should update now or wait and maybe do again after PR...

from opencm9.04.

@KurtE,
Sorry for my late answer.

I think it is necessary to do PR again.
It would be nice to proceed with the part that is being discussed with @kijongGil in OpenCR.

from opencm9.04.

@OpusK ,

No problem, I know you are busy. May be a couple of days, before I can update the PR. And make sure that the code is in sync with the stuff in OpenCR and Dynamixel SDK and Workshop.

from opencm9.04.

@KurtE,

I want to close this issue because #53 has been merged. are you okay?

from opencm9.04.