PR #184 - STM32F1 USART Improvements

What could be included in further releases, or for the forum.
edogaldo
Posts: 252
Joined: Fri Jun 03, 2016 8:19 am

PR #184 - STM32F1 USART Improvements

Post by edogaldo » Sun Jul 10, 2016 10:49 am

Dear all,
I'd like to draw attention to subject PR (https://github.com/rogerclarkmelbourne/ ... 2/pull/184) which provides following USART improvements in the F1 library:
  • Buffered interrupt-based TX: as most of you know F1 library is using a polling TX approach which has very slow performance (even worse than in the UNO platform), buffered interrupt-based TX significantly increase the TX performance
  • Runtime level RX/TX dynamic buffer allocation: generally Arduino based libraries use a static buffer allocation approach, this implies allocating buffers for all devices even if they are not used; runtime level dynamic buffer allocation allows instead allocating buffer for the only devices effectively used and also allows users to easily change the buffers size as preferred
  • Introduction of non-blocking TX: this feature, taken from the F4 USBSerial library, allow users to specify they don't need blocking TX; difference between blocking TX and non-blocking TX is like difference between TCP and UDP protocols: in the first case the system will engage to guarantee that the full message is sent, in the second case the system will send as much of the message as possible (this depends on the TX buffer size) and will discard the remaining part
Well, since this PR is introducing somewhat significant changes in the repository, its inclosure will probably depend on how much consensum it will get so this thread is to:
  • invite users to actively test the changes and provide feedbacks
  • evaluate the forum members interest on the improvements proposed
Thanks in advance and best regards, E.

PS: just want also to highlight that this PR doesn't aim to remove any of the existing functionalities (like RX buffering) and that effort has been spent to guarantee backward compatibility

edogaldo
Posts: 252
Joined: Fri Jun 03, 2016 8:19 am

Re: PR #184 - STM32F1 USART Improvements

Post by edogaldo » Mon Jul 11, 2016 9:24 pm

Quoting @mrburnette from: viewtopic.php?f=3&t=1189&p=15811#p15808
How thoroughly is your serial code tested? Any test cases that fail as the F1xx is currently constructed that pass flawlessly with your changes? In lieu of test code, how would you propose your changes to be tested? Are there any dependencies introduced in your proposal (limitations, reserved resources, known bombs)?
I'm using this test sketch:

Code: Select all

//#define Serial Serial1  // TX A2

#include <libmaple/scb.h>
byte nodeID = 10;
byte packetNum = 11;
float C = 42.611;
float F = 42.611;
float H = 42.6;
float vBatt = 3.14;
int packetLoss = 10;
int packetTotal = 10000;

#define tx_buf_start 1
#define tx_buf_stop 300
uint16 tx_buf_size=tx_buf_start;


void printData() {
  Serial.print(nodeID);
  Serial.print(",");
  Serial.print(packetNum);
  Serial.print(",");
  Serial.print(C);
  Serial.print(",");
  Serial.print(F);
  Serial.print(",");
  Serial.print(H);
  Serial.print(",");
  Serial.print(vBatt,3);
  Serial.print(",");
  Serial.print(packetLoss);
  Serial.print(",");
  Serial.println(packetTotal);
  //Serial.println(SCB_BASE->VTOR,HEX);
  //Serial.println("abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789");
}

void setup() {
  //Serial.begin(57600);
}

void loop() {
  //Serial.beginNew(9600,tx_buf_size);
  //Serial.beginNew(57600,tx_buf_size);
  Serial.beginNew(115200,tx_buf_size);
  //Serial.disableBlockingTx();
  unsigned long start = micros();
  printData();
  unsigned long end = micros();
  Serial.print("tx_buf_size[");Serial.print(tx_buf_size);Serial.print("]: ");Serial.print(end - start);Serial.println(" us");
  Serial.println();
  tx_buf_size *= 2;
  if(tx_buf_size > tx_buf_stop){
    tx_buf_size = tx_buf_start;
    //Serial.println("=================");
    Serial.println("@@@@@@@@@@@@@@@@@@@");
    Serial.println();
  }
  delay(1000);
}
I anyway tested it also with some other Arduino standard USART sketches and my implementation worked as much as the original implementation, I mean: in the cases where my code didn't work, neither did the original one..

About dependencies, reserver resources or known bombs, at the moment I can list:
  • possible heap fragmentation if the same USART device is initialized more than once with different buffer sizes
  • at the moment I don't handle (looking for suggestions) the case in which malloc() "fails" returning NULL
  • at the moment I'm not posing limits to the maximum buffer size, which is uint16 (so up to 65535 bytes): I accept suggestions on whether/which should be a fair limit
  • @RickKimball noted me I didn't check the TX buffer empty in the Serial.end(): have to fix this
At the moment I'm not aware of any further drawbacks.

Best, E.
Last edited by edogaldo on Mon Jul 11, 2016 10:58 pm, edited 3 times in total.

User avatar
Rick Kimball
Posts: 933
Joined: Tue Apr 28, 2015 1:26 am
Location: Eastern NC, US
Contact:

Re: PR #184 - STM32F1 USART Improvements

Post by Rick Kimball » Mon Jul 11, 2016 10:00 pm

If you have an interest in the implementation of the usart code, please look the pull request and comment on github.

It seems like with such significant changes, there needs to be a test sketch that thoroughly exercises all the edge cases for all the SerialX ports and does some kind of automated verification maybe using the serial ports wired to each other.

I personally would prefer that the ring_buffer implementation was left unchanged.

-rick
-rick

edogaldo
Posts: 252
Joined: Fri Jun 03, 2016 8:19 am

Re: PR #184 - STM32F1 USART Improvements

Post by edogaldo » Mon Jul 11, 2016 10:10 pm

Rick Kimball wrote:(...)
I personally would prefer that the ring_buffer implementation was left unchanged.
For sake of knowledge: I also changed the ring_buffer implementation (the implementation, not the interface) to leverage the full buffer size and not ([buffer size] - 1) as in the current implementation.
Assuming users want to define a small buffer, 1 byte can matter: i.e. on an overall 8 bytes buffer, 1 byte is 12,5%..

edogaldo
Posts: 252
Joined: Fri Jun 03, 2016 8:19 am

Re: PR #184 - STM32F1 USART Improvements

Post by edogaldo » Mon Jul 11, 2016 10:26 pm

It seems like with such significant changes, there needs to be a test sketch that thoroughly exercises all the edge cases for all the SerialX ports and does some kind of automated verification maybe using the serial ports wired to each other.
You are right but I don't think it can be thoroughly tested with just one sketch, this is why I asked for support in testing and providing feedbacks.
The more people tests it, the more issues can be discovered.

Anyway referred to issues, just want to say the tests should first of all focus to regressions, that mean things that in the original implementation worked fine and in the new implementation don't work anymore.
It's not aim of this PR to fix possible existing issues (at least not now).

User avatar
mrburnette
Posts: 1794
Joined: Mon Apr 27, 2015 12:50 pm
Location: Greater Atlanta
Contact:

Re: PR #184 - STM32F1 USART Improvements

Post by mrburnette » Mon Jul 11, 2016 11:31 pm

Assuming users want to define a small buffer, 1 byte can matter: i.e. on an overall 8 bytes buffer, 1 byte is 12,5%..
Ok, this is simply uncalled for, IMO, and is borderline contempt of our collective intelligence. We can do the math.

Ray

Edited:
It's not aim of this PR to fix possible existing issues (at least not now).
I come from a rather strict work ethic (supervision of 10 Java coders in a $600M software capital investment in a message queing environment) and I can state that (significant) issues found in unit testing based on prior code were corrected prior to an enhancement. Non-significant issues caught in unit testing were incorporated into the enhancement. Non-significant here is meant to imply no budget or implemention date slippage. Non-significant issues caught in unit testing which did cause budget/date slippage were prioritized as hot-fixes to be applied with a change request window from the BU customer.

You make it sound like you only want your stuff injected into the core and anything else is someone's elses problem. Having had a few discussions with you, I really do not think this is what you mean.

If we find something in testing, we will democratically decide how and when it should be fixed. So far, there has not been a shortage of talent to work issues... although Roger often over volunteers himself, IMO.
Last edited by mrburnette on Mon Jul 11, 2016 11:57 pm, edited 2 times in total.

User avatar
Rick Kimball
Posts: 933
Joined: Tue Apr 28, 2015 1:26 am
Location: Eastern NC, US
Contact:

Re: PR #184 - STM32F1 USART Improvements

Post by Rick Kimball » Mon Jul 11, 2016 11:42 pm

I would prefer to see this pull request split into 2 pull requests. One that deals with the failing of our current implementation to use interrupts and a buffered tx method. The second to address the ability to dynamically change the the size of the rx and tx buffer. I can imagine I would have no objections to the first one and lots of problems with the second.

-rick
-rick

User avatar
mrburnette
Posts: 1794
Joined: Mon Apr 27, 2015 12:50 pm
Location: Greater Atlanta
Contact:

Re: PR #184 - STM32F1 USART Improvements

Post by mrburnette » Mon Jul 11, 2016 11:55 pm

Rick Kimball wrote:I would prefer to see this pull request split into 2 pull requests. One that deals with the failing of our current implementation to use interrupts and a buffered tx method. The second to address the ability to dynamically change the the size of the rx and tx buffer. I can imagine I would have no objections to the first one and lots of problems with the second.

-rick

I can appreciate that viewpoint, but I wish it had surfaced earlier before e's code was completed. Now, it requires extra effort to conform to your request.

I think we should test and vote on what has been created - give it our best. If issues are found, let's see where things break and make suggestions at that time. However, I promised to not vote, but I can still have procedural input.

Ray

User avatar
mrburnette
Posts: 1794
Joined: Mon Apr 27, 2015 12:50 pm
Location: Greater Atlanta
Contact:

Re: PR #184 - STM32F1 USART Improvements

Post by mrburnette » Tue Jul 12, 2016 12:03 am

Rick Kimball wrote: <...> all the edge cases for all the SerialX ports and does some kind of automated verification maybe using the serial ports wired to each other.
<...>
-rick
Perhaps two STM32 boards tied:
Rx ----> Tx
Tx ----> Rx

I think this is more realistic than a local loopback. \

Ray

edogaldo
Posts: 252
Joined: Fri Jun 03, 2016 8:19 am

Re: PR #184 - STM32F1 USART Improvements

Post by edogaldo » Tue Jul 12, 2016 7:40 am

mrburnette wrote: You make it sound like you only want your stuff injected into the core and anything else is someone's elses problem. Having had a few discussions with you, I really do not think this is what you mean.

If we find something in testing, we will democratically decide how and when it should be fixed. So far, there has not been a shortage of talent to work issues... although Roger often over volunteers himself, IMO.
Well, I'm not paid for this work so I think my contribution can be considered demonstration of good will, else I could just have kept it for myself.
As said I'm interested in improving the library and I have no issues in fixing also existing bugs (on a best effort basis) as it is for sure improving the library; just want to say it would be unfair to debt to my changes existing bugs..

@Rick Kimball
About the ring buffer implementation changes: before making my changes I investigated the Arduino UNO and DUE implementations, what I found was that they are significantly different not only respect to libmaple implementation but also respect to each other so it seems to me there's no that much agreement on a standard ring buffer implementation; furthermore, looking at the specific changes, they don't seem to me so much scary and the modifications seem to me even more clear anyway, as said in Github, I can revert back to the existing version if really preferred.

Also about dynamic buffering, as already said in another thread, I started working on a minimal solution for buffered interrupt-based TX which I posted here: viewtopic.php?f=3&t=1189&p=15297#p15297
(note this implementation probably still needs some change)
Then, looking at the thread evolution, it seemed to me it would have been a nice plus to also add dynamic buffering and blocking/non-blocking TX so I then decided to propose everything in a unique PR.

Finally, we can evaluate restarting from above solution if really preferred.


Best, E.
Last edited by edogaldo on Tue Jul 12, 2016 4:51 pm, edited 1 time in total.

Post Reply

Who is online

Users browsing this forum: No registered users and 1 guest