Modify UART Class to Make Use of the txBuffer #304

delta-G · 2024-05-06T06:18:36Z

I opened this to replace #303 because it had a bunch of files included from submodules. This should be cleaner. Sorry for the mess.

This pull request makes changes in Serial.h and Serial.cpp so that the UART class will make use of the TxBuffer. This greatly reduces the amount of time that Serial.print will block, especially for long strings. Currently the class makes no use of the 512 bytes it has reserved for a txBuffer.

I've changed the write functions so that they will load characters into the buffer and start a transmission if one is not already going.

Because the HAL takes a pointer to the data, I added a char variable to read into so the buffer can be advanced. I cannot attempt to send a larger portion of the buffer because the HAL expects contiguous memory and it will break in the case where the string to send rolls over the end of the ring buffer. I will submit another PR later to deal with that if I find a good way.

While it is possible to submit an entire string to the HAL at once, the string is submitted by pointer so it creates a possible race if there is some other code that changes the string before it gets fully printed. By utilizing the txBuffer, this problem is also alleviated.

I have tested this code in several configurations and it appears to work just dandy. Please let me know what you think.

delta-G · 2024-05-06T21:17:03Z

I pushed another commit that adds the availableForWrite function.

I also noticed that the original commit on this PR also fixes flush. The original code had a busy wait for txBuffer.available() which would always return false because the txBuffer wasn't being used. With the code in this PR flush should now work.

paulvha · 2024-05-25T16:46:56Z

I have tried this change because it makes complete sense and also because I am having issues with the Sparkfun RA6M5 Serial1 port. I have copied the changes as indicated and I had issue :

txc in write() was not defined. It is defined in wrappercallback() as : static char txc; I moved that definition global to solve it.

It then compiled and worked without a problem.

FYI The problem was not solved on the RA6M5 with this change..the first character received is somehow 0x0 instead of the expected 0x7E. It is sent by the device as monitored and detected with an external tracer (Saleae). Also the sketch does work unmodified with a UNOR4 Wifi. Different issue

delta-G · 2024-05-26T03:23:55Z

I have tried this change because it makes complete sense and also because I am having issues with the Sparkfun RA6M5 Serial1 port. I have copied the changes as indicated and I had issue :

txc in write() was not defined. It is defined in wrappercallback() as : static char txc; I moved that definition global to solve it.

It then compiled and worked without a problem.

FYI The problem was not solved on the RA6M5 with this change..the first character received is somehow 0x0 instead of the expected 0x7E. It is sent by the device as monitored and detected with an external tracer (Saleae). Also the sketch does work unmodified with a UNOR4 Wifi. Different issue

txc is defined as a member variable of the UART class. I did this because a global would be shared between instances and I didn't want to introduce issues if two UARTs were working at the same time. There is another txc defined in the interrupt callback as a local variable, but it is only used in the context of the handler. That's because the handler doesn't belong to any one instance.

paulvha · 2024-05-26T08:05:15Z

Thanks. My error as I missed copying the txc definition from the header file.

angavrilov · 2024-06-26T22:59:35Z

cores/arduino/Serial.cpp

-        uart_ptr->tx_done = true;
+          if(uart_ptr->txBuffer.available()){
+              static char txc;
+              txc = uart_ptr->txBuffer.read_char();


Is it OK to use a static variable here instead of uart_ptr->txc? Would this work correctly if there are multiple uarts working simultaneously?

That's a good question. I made it static because it was being passed as a pointer on the next line but I didn't really think about it being there in multiple instances.

I think that it will be OK because R_SCI_UART_Write is going to use that data one line later and then we're done with it. I think it would be OK to make it not static.

I wish I could grab a whole chunk of the tx_buffer there so the code in r_sci_uart.c could make effective use of the FIFO. The problem is that I would need to know where the data in the tx_buffer wraps around or I could run off the end.

I have a separate branch that uses DMA and it doesn't have this problem because I set the DMA extended repeat area to match the tx_buffer and the DMA controller will automatically wrap back around. But that code seems to be broken in other places.

Actually, looking at it now, I don't see why I don't just use uart_ptr->txc instead. Maybe I thought because it was an interrupt handler that I wouldn't have access but I do.

I'll update the branch tonight and retest a little.

I updated the code to use the member variable instead. It tests ok. I did a lot of printing without any failures. Unfortunately I seem to have messed up my branch.

Updated once more to load characters 16 at a time in the interrupt callback. This takes advantage of the FIFO to reduce the number of interrupts.

delta-G · 2024-06-29T23:33:43Z

OMG! What did I do wrong? Where did all those other commits come from?

What do I do? @per1234 can you help?

per1234 · 2024-06-30T04:17:15Z

Hi @delta-G.

Start by doing a hard reset of the branch back to the last commit (9b439c5) before the unwanted commits were introduced:

$ git checkout txBuffer

Switched to branch 'txBuffer'

$ git reset --hard 9b439c5f830b797cb77f774c3d7cc8b2c245c03e

HEAD is now at 9b439c5 added availableForWrite

Now all the unwanted commits have been removed from the branch, but the recent commit you want (2832d3d) was also removed. So you need to cherry-pick that one back onto the branch:

$ git cherry-pick 2832d3d5e8c7758235b29c286abf35dc26d07a4f

[txBuffer acfc357] Use member variable in ISR instead of static veriable
 Author: david <[email protected]>
 Date: Sat Jun 29 18:29:01 2024 -0500
 1 file changed, 2 insertions(+), 3 deletions(-)

Now you need to force push the branch to your fork on GitHub:

$ git push --force

⚠ You should never do a force push to a production branch in a repository since this will cause the branch's history to be different from that in clones of the repository, which will be a bad time for project collaborators. However, it is fine to do so in branches that are being used to stage commits as is the case with a PR branch. GitHub will even automatically add a helpful note to the pull request thread to make it clear what happened.

If you want to bring your branch up to date with the main branch, you can perform a rebase, which will put the commits that are staged in your branch on top of the history in main:

$ git rebase main

Successfully rebased and updated refs/heads/txBuffer.

Then force push the updated branch to your fork on GitHub:

$ git push --force

delta-G · 2024-06-30T20:04:49Z

Thank you @per1234 for the detailed instructions. You even had the hashes in there for me so I didn't have to look in two places. Nobody does it like you do.

delta-G · 2024-06-30T20:18:28Z

Here's some test code for a WiFi that just print alternating lines of Lewis Carroll to both Serial and Serial1. It performs as expected. I also ran a test where I was echoing Serial to Serial1 and vice versa and it worked fine for that.

void setup() {

  Serial.begin(115200);
  Serial1.begin(115200);
  Serial.println("\n\n *** " __FILE__ " ***\n\n");

}

void loop() {

  Serial.println("Twas brillig and the slithy toves did gyre and gimbel in the wabe.  All mimsy were the borogoves and the mome raths outgrabe.");
  Serial1.println("Beware the Jabberwock my son.  The jaws that bite.  The claws that catch.  Beware the Jubjub bird and shun the frumious Bandersnatch.");
  delay(200);
  Serial.println("He took his vorpal sword in hand.  Long time the manxome foe he sought.  So rested he by the Tumtum tree and stood a while in thought");
  Serial1.println("And as in uffish thought he stood the Jabberwock with eyes of flame came whiffling through the tulgey wood and burbled as it came.");
  delay(200);
  Serial.println("One two one two and through and through the forpal blade went snicker snack.  He left it dead and with its head he went galumphing back.");
  Serial1.println("And hast thous slain the Jabberwock?  Come to my arms my beamish boy.  Oh frabjous day callook callay he chortled in his joy.");
  delay(200);

}

delta-G · 2024-07-03T05:13:07Z

I added one more commit. I moved a few things around in UART::write but didn't really change anything there. I changed the txc member to an array and changed the interrupt callback code to load characters 16 at a time to take advantage of the FIFO.

workgroupengineering · 2024-07-24T09:38:12Z

Hi, is there an ETA of when this PR will be merged and published?

delta-G changed the title ~~Use txBuffer~~ Modify UART Class to Make Use of the txBuffer May 6, 2024

per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels May 6, 2024

angavrilov reviewed Jun 26, 2024

View reviewed changes

delta-G force-pushed the txBuffer branch from 2832d3d to a6ec0b8 Compare June 30, 2024 20:01

delta-G added 4 commits June 30, 2024 15:01

Use txBuffer

c180836

fixed formatting

b4a4e37

added availableForWrite

e975966

Use member variable in ISR instead of static veriable

2878c5a

delta-G force-pushed the txBuffer branch from a6ec0b8 to 2878c5a Compare June 30, 2024 20:02

Loads characters 16 at a time in the interrupt callback

0a14d85

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify UART Class to Make Use of the txBuffer #304

Modify UART Class to Make Use of the txBuffer #304

delta-G commented May 6, 2024

delta-G commented May 6, 2024

paulvha commented May 25, 2024 •

edited

Loading

delta-G commented May 26, 2024

paulvha commented May 26, 2024 •

edited

Loading

angavrilov Jun 26, 2024

delta-G Jun 27, 2024

delta-G Jun 27, 2024

delta-G Jun 29, 2024

delta-G Jul 3, 2024

delta-G commented Jun 29, 2024 •

edited

Loading

per1234 commented Jun 30, 2024

delta-G commented Jun 30, 2024

delta-G commented Jun 30, 2024

delta-G commented Jul 3, 2024

workgroupengineering commented Jul 24, 2024

Modify UART Class to Make Use of the txBuffer #304

Are you sure you want to change the base?

Modify UART Class to Make Use of the txBuffer #304

Conversation

delta-G commented May 6, 2024

delta-G commented May 6, 2024

paulvha commented May 25, 2024 • edited Loading

delta-G commented May 26, 2024

paulvha commented May 26, 2024 • edited Loading

angavrilov Jun 26, 2024

Choose a reason for hiding this comment

delta-G Jun 27, 2024

Choose a reason for hiding this comment

delta-G Jun 27, 2024

Choose a reason for hiding this comment

delta-G Jun 29, 2024

Choose a reason for hiding this comment

delta-G Jul 3, 2024

Choose a reason for hiding this comment

delta-G commented Jun 29, 2024 • edited Loading

per1234 commented Jun 30, 2024

delta-G commented Jun 30, 2024

delta-G commented Jun 30, 2024

delta-G commented Jul 3, 2024

workgroupengineering commented Jul 24, 2024

paulvha commented May 25, 2024 •

edited

Loading

paulvha commented May 26, 2024 •

edited

Loading

delta-G commented Jun 29, 2024 •

edited

Loading