ArduinoCore-renesas icon indicating copy to clipboard operation
ArduinoCore-renesas copied to clipboard

Modify UART Class to Make Use of the txBuffer

Open delta-G opened this issue 1 year ago • 10 comments

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 avatar May 06 '24 06:05 delta-G

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.

delta-G avatar May 06 '24 21:05 delta-G

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 :

  1. 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

paulvha avatar May 25 '24 16:05 paulvha

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 :

  1. 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.

delta-G avatar May 26 '24 03:05 delta-G

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

paulvha avatar May 26 '24 08:05 paulvha

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

What do I do? @per1234 can you help?

delta-G avatar Jun 29 '24 23:06 delta-G

Hi @delta-G.

Start by doing a hard reset of the branch back to the last commit (9b439c5f830b797cb77f774c3d7cc8b2c245c03e) 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 (2832d3d5e8c7758235b29c286abf35dc26d07a4f) 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

per1234 avatar Jun 30 '24 04:06 per1234

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 avatar Jun 30 '24 20:06 delta-G

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 avatar Jun 30 '24 20:06 delta-G

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.

delta-G avatar Jul 03 '24 05:07 delta-G

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

workgroupengineering avatar Jul 24 '24 09:07 workgroupengineering