Support for Part separator symbols added

[nmzaheer]

Part separator symbols as per SerialPlotter protocol i.e. Comma Space and Tab characters have been added.

This resolves #15

Please review and verify.

[per1234]

Please update the comment here:

arduino-serial-plotter-webapp/src/msgAggregatorWorker.ts

Lines 70 to 72 in eac6d39

	//there are two supported formats:
	// format1: <value1> <value2> <value3>
	// format2: name1:<value1>,name2:<value2>,name3:<value3>

[per1234]

There is a regression here for a use case that is not explicitly supported by the specification, but is supported by the Arduino IDE 1.x Serial Plotter and previous implementation of this project. That use case is the combination of a trailing delimiter and \r\n separator.

For example:

void setup() {
  Serial.begin(9600);
}
void loop() {
  for (byte value = 0; value < 4; value++) {
    for (byte variableOffset = 0; variableOffset < 3; variableOffset++) {
      Serial.print(value + variableOffset);
      Serial.print('\t');
    }
    Serial.println();
  }
  delay(100);
}

You can see how code that generates data via a for loop favors a trailing delimiter, and of course Serial.println(); is a convenient way to produce a separator.

Previously, that sketch produced 3 variables as expected:

Arduino IDE 1.8.19:

arduino-serial-plotter-webapp@eac6d39 / [email protected] / Arduino IDE 2.0.0-rc9.3:

After the change made here, it produces 4 variables:

The problem is the \r in the \r\n separator produced by Serial.println(); is being treated as a separate value. Previously, that \r was consumed by message.split(/\s/); (because it matches /\s/).

[nmzaheer]

What if the separator regex is modified to include the carriage return character \r? Would that solve the problem or am I still missing something ?

const separator = "\r?\n";

Is there a particular reason why Serial.println outputs a \r\n and not \n ? We cannot have two different conventions for EOL character. Either we amend the SerialPlotter protocol or drop support for \r\n entirely.

[per1234]

const separator = "\r?\n";

That general approach should work. However, separator is used in the code directly as a string in addition to its use in the definition of separatorRegex, so the implementation is not quite so simple.

It will need to be something like this (not tested):

diff --git a/src/msgAggregatorWorker.ts b/src/msgAggregatorWorker.ts
index b05dddf..913915b 100644
--- a/src/msgAggregatorWorker.ts
+++ b/src/msgAggregatorWorker.ts
@@ -20,7 +20,7 @@ let buffer = "";
 let discardFirstLine = true;
 const separator = "\n";
 const delimiter = "[, \t]+"; // Serial Plotter protocol supports Comma, Space & Tab characters as delimiters
-var separatorRegex = new RegExp(`(${separator})`, "g");
+var separatorRegex = new RegExp(/(\r?\n)/, "g");
 var delimiterRegex = new RegExp(`(${delimiter})`, "g");
 
 export const parseSerialMessages = (
@@ -55,7 +55,7 @@ export const parseSerialMessages = (
   // remove the previous buffer
   buffer = "";
   // check if the last message contains the delimiter, if not, it's an incomplete string that needs to be added to the buffer
-  if (messagesAndBuffer[messagesAndBuffer.length - 1] !== separator) {
+  if (!separatorRegex.test(messagesAndBuffer[messagesAndBuffer.length - 1])) {
     buffer = messagesAndBuffer[messagesAndBuffer.length - 1];
     messagesAndBuffer.splice(-1);
   }
@@ -65,7 +65,7 @@ export const parseSerialMessages = (
 
   // for each line, explode variables
   messagesAndBuffer
-    .filter((message) => message !== separator)
+    .filter((message) => !separatorRegex.test(message))
     .forEach((message) => {
       const parsedLine: { [key: string]: number } = {};

We cannot have two different conventions for EOL character.

Why not? Inconsistency in EOL is a common situation. Not so long ago, Windows, Linux, and macOS each used a different EOL.

Is there a particular reason why Serial.println outputs a \r\n and not \n ?

No idea. It has been that way since 2006, and in the proud tradition of Arduino the person who made the change didn't bother to explain the reason:

arduino/Arduino@650c275

amend the SerialPlotter protocol

I think it is a good idea. The protocol already does specify that \r\n is supported by using Serial.println() in the all the example programs, but that is too vague. It should be stated explicitly that both are supported.

[per1234]

drop support for \r\n entirely.

It would be too disruptive. I'm certain the code can be made to support either without any real difficulty.

The only real challenge here is that there is no test coverage in this project, despite the fact that this code is very easy to write unit tests for.

[nmzaheer]

I would like to contribute by writing the tests but I still haven't managed to setup my dev environment right.

Ideal case, I would like to debug the modifications before committing it to the repo. However, I have not been able to interface my Arduino with the standalone web-app or write a websocket script to feed values to the web-app.

Currently, I've been modifying the randomValueGenerator code manually to feed various types of messages and check its results. If you could guide me in setting this up, I would be able to contribute a lot more.

[nmzaheer]

const separator = "\r?\n";

That general approach should work.

Do we have to support \r as an EOL character? In that case, the regex would be

const separator = "\r?\n?";

[per1234]

Do we have to support \r as an EOL character?

No. It has never been supported and I haven't seen any complaints about the lack of support (I think it is not a very common separator in Arduino sketches despite being supported in the line ending menu of Serial Plotter and Serial Monitor)

For example, this sketch:

void setup() {
  Serial.begin(9600);
}
void loop() {
  for (byte value = 0; value < 4; value++) {
    for (byte variableOffset = 0; variableOffset < 3; variableOffset++) {
      Serial.print(value + variableOffset);
      Serial.print('\t');
    }
    Serial.print('\r');
  }
  delay(100);
}

Produces no output in the plotter:

[email protected]

Arduino IDE 1.8.19

[nmzaheer]

Hi @per1234 Did you have a chance to review this code ?

Is there something pending from my side?

[per1234]

Hi @nmzaheer. Sorry, I have not been able to make time to review this yet. I will get to it as soon as possible.

I will also try to provide a response to your question about how the project can be tested (I'm actually not very familiar with the internals of this project so I don't have an answer for you off hand).

[per1234]

Hi @nmzaheer. I wrote some tests and ran them against this PR. Things are looking very good, but there is one regression compared to the version of the plotter from the main branch of this repository (eac6d39).

It occurs under the following conditions:

• Single field
• No trailing field delimiter
• \n record delimiter

This is a demonstration sketch:

void setup() {
  Serial.begin(9600);
}

void loop() {
  for (byte value = 0; value < 3; value++) {
    Serial.print(value);
    Serial.print('\n');
    delay(100);
  }
}

The plotter version at eac6d39 produces the expected chart:

The plotter version from this PR (ff372c0) produces this odd chart:

---

I submitted a pull request to your fork for adding the tests in case that will be useful to you in resolving this issue: nmzaheer#1

You can run the tests with the command:

npm run-script test

Please let me know if you have any questions.

[nmzaheer]

The tests were failing because the RegExp index was not reset before checking the string. Now, all tests are passed.

Also, I think I fixed all indentation issues.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@eac6d39). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #16   +/-   ##
=======================================
  Coverage        ?   87.71%           
=======================================
  Files           ?        1           
  Lines           ?       57           
  Branches        ?       12           
=======================================
  Hits            ?       50           
  Misses          ?        3           
  Partials        ?        4           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[per1234]

Thanks so much @nmzaheer!