Refactor HyperSerialEsp

[Lord-Grey]

Refactor HyperSerialEsp8266

(@asturel might share additional feedback on improved user experience)

Changes

• Split serial processing from content validation and LED updates

• Move to non-blocking serial functions

• Do Flechter calculations/validation only for a frame and not the "garbage" before frames, i.e. incomplete frames

• Enable/Disable statistics and define reporting interval allows exploring a good balance between fps and error rate

• Have statistics as a one-liner to show it in the HyperHDR/Hyperion log as one record
  (corresponding backed code Serial LED-Devices - Support device feedback, show statistics provided

• More granular statistics breakdown, showing the different bad frame scenarios
  FPS: Updates to the LEDs per second
  F-FPS: Frames identified per second
  S: Shown (Done) updates to the LEDs per given interval
  F: Frames identified per interval (garbled grames cannot be counted)
  G: Good frames identified per interval
  B: Total bad frames of all types identified per interval
  BF: Bad frames identified per interval
  BS: Skipped incomplete frames
  BC: Frames failing CRC check per interval
  BFL Frames failing Fletcher content validation per interval

• Allow to test serial processing without strip processing (undefine ENABLE_STRIP) using 2nd serial

Fixes

• Consider disabling calibration when update package changes from calibration to non-calibration (A?A -> A?a)

PS

Other sketches can be provided when PR is accepted

Sample Statistics

200 LEDs, Feed 50Hz, ESP8266 for illustration

[awawa-dev]

Honestly, I announced my upcoming HyperHDR refactoring with some recent major changes earlier (awawa-dev/HyperHDR#379). Next, one day HyperHDR v19beta that includes that change is released for testing , the next day this PR, so to avoid further misunderstandings on the next day I released my changes. From serious refactoring, I expected far-reaching changes so that it would justify the need to re-test it on all my ESP platforms.

For such a change I consider migration to PlatformIO where the Arduino IDE looks like a tool from the previous era (although the Arduino IDE still has some advantages, but we do not use them in this project, so there could only be one decision), which already makes this PR incompatible, grouping variables and functions while maintaining the lightness of the project, unifying the project into one source file shared by all types of LEDs, removing unnecessary floating-point operations, which is an unnecessary burden for ESP, even if calibration is not performed often, using new PlatformIO functions such as unit-testing to test new algorithms its enumeration to maintain backward compatibility (which makes the second diagnostic serial port for testing it manually unnecessary). And of course using the new possibilities of HyperHDR v19. When it comes to logging, it has always been an important part of HyperSerialEsp8266, but due to the observations of ESP and RPI in practice (other such unexpected curiosities forced me to create HyperSPI or at least gave me additional incentive to create the fastest external driver for generic ESP and Rpi) port for HyperHDR communication with HyperSerialESP8266 / ESP32 is and will be used only as WriteOnly and will output the data only is there is no data incoming.

I completely don't understand what you mean with blocking something (Move to non-blocking serial functions) ? We only need a minimal amount of resources to process the data that came into the buffer (your PR also "blocks" ESP in this sense) and we pass control back, otherwise we run a different risk: hardware watchdog. Anyway, look at the definitions https://docs.arduino.cc/built-in-examples/communication/SerialEvent , the function you used in your PR. You operate the same on the main loop but with one very important difference introducing a serious drawback: in your PR if ESP was not ready to display the frame it will do it's only when the next data arrives on the serial port. As a result, it may e.g immediately render 2 frames causing blinking / non-smooth transition, or it will not be possible to render a new frame and delay it until the next cycle. In HyperSerialEsp8266, the loop will try to render the cached frame even if no new data arrives, it is enough for the ESP resources to free up.

[awawa-dev]

To clarify one thing: I can see you're also calling ShowMe from the main loop also outside of the main serial port procedure, but this proves that the code has become confusing due to duplication of such an important call in different parts of the source code.

[Lord-Grey]

Hi @awawa-dev
Thanks for taking the time having a look at the provided PR and digesting the proposed changes.
Sometimes different parallel development just "cross" each other where I agree that moving to platform.io is/was the next logical step. As I just spend some time with the sketch, I did not went that route and took the sketch available at that time as a basis.

Nevertheless, let me give you some background of the applied changes, as I was not following the statements on "confusing due to duplication", and the PR description was maybe not too elaborate on the rationale.

The updated sketch separate the two concern of reading serial data vs. validating and displaying the data.
In fact, the serial processing is not intermingled with showing LED updates, i.e. there is only one place of ShowMe in the code.
At best that allows for better multi-core processing, but was not able to test this given I have no ESp32 available.
In terms of "blocking" there is no need to spend to much time on argue here. readBytes is "blocking" as it has a timeout where read immediately return.s As you set the timeout = 50ms, it behaves kind of non-blocking; just ignore the statement in the PR...

In term of drawback on updates, the sketch just follows a different strategy. I take the risk of missing updates to always have the latest updates at hand. In case I would show the latest update, the next good updated would not be displayed given the LED update is still busy.
As we are seeding with high update speed the benefit is higher to be in sync rather doing updates retrospectively.
Anyway, the users would better be educated reducing fps or increase the latchtime /refreshtime when the "no-shows" increase.
There is no benefit streaming with higher rates, if the LEDs' physic or library cannot cope with update frequency.

In case you find one or the other aspect still helpful, I might also align the code to platform.io, but I would leave it to you giving me a go :) ...
We should not get into a PR back and forth....

[awawa-dev]

Hi

In terms of "blocking" there is no need to spend to much time on argue here. readBytes is "blocking" as it has a timeout where read immediately return.s As you set the timeout = 50ms, it behaves kind of non-blocking; just ignore the statement in the PR...

To clarify it further. First we check the available incoming bytes to read using Serial.available(), then readBytes reads only the already available data in non blocking way. timeout has nothing to do here. This is non blocking approach!

uint16_t internalIndex = min(Serial.available(), MAX_BUFFER);
 if (internalIndex > 0)
		internalIndex = Serial.readBytes(buffer, internalIndex);

In term of drawback on updates, the sketch just follows a different strategy. I take the risk of missing updates to always have the latest updates at hand. In case I would show the latest update, the next good updated would not be displayed given the LED update is still busy.
As we are seeding with high update speed the benefit is higher to be in sync rather doing updates retrospectively.
Anyway, the users would better be educated reducing fps or increase the latchtime /refreshtime when the "no-shows" increase.
There is no benefit streaming with higher rates, if the LEDs' physic or library cannot cope with update frequency.

Oh yes. I always point that out in HyperHDR manuals. It can be easily tested using continues output with high refresh rate and read the esp statistics when configuring the setup: it's one time job. There is no latch time anymore in HyperHDR and refresh time should be set to zero as per manual.

At best that allows for better multi-core processing, but was not able to test this given I have no ESp32 available.

Currently ESP32 version is locked on Arduino 1.0.6 (and that unfortunately affects neopixelbus library version also, btw platformio can enforce proper version , arduino IDE can't since its configuration is based purely on user own action) due to some issues with version 2.x Probably further improvements and features like multi-segment or use proper multi thread handling must wait to see, if it can be resolved: quite uncomfortable situation especially when arduino esp8266 doesn't have such problems and we can't use newer esp32 boards that require Arduino 2.0. I've already submitted a ticket in arduino-esp32 repo, because it may affect also other projects that communicate using serial protocol not only at 2Mb speed like wled in wire configuration. Thanks to some initial feedback from arduino-esp32 there is one improvement that can be made in my project and it will be submitted soon, but still the performance is degraded.

Overall, I don't see any benefits that this PR would bring to this project in terms of better performance, stability or new features. The reasons behind migrating to PlatformIO and related improvements were well explained at #18. When it comes to the PlatformIO there are many clear advantages, and none to justify staying with the Arduino IDE.

[awawa-dev]

irrelevant after migrating to platformio