Fast Array Write for Atomic Type Arrays

[suahelen]

Pull Request Details

This PR contains a few changes. First it should solve some name clash conflicts with the print.tcc that was renamed to rpcPrint.tcc. They arose when I used the lib with platformio.

The Larger changes revolve around the handling of larger arrays. Arrays now have the signature of [] for dynamic typed arrays as before. If an array / vector is of type atomic (definition in atomic_types.tcc it will be written with {}. This differentiation allows the client to directly read or write an entire byte array instead of type checking writing each value separately.

This PR also adds the Rolling_buffer<T, s> Type. A useful type of array for streaming data from a device.

Breaking Changes

If an array now uses an atomic type it will be written to the stream with {} This might break stuff if the client side is not updates. I will have a PR to match that there.

[jfjlaros]

Thank you for your suggestions.

Before I consider this PR in more detail, perhaps you can help me understand the ideas behind it a bit better.

dynamic typed arrays

This library does not support dynamically typed arrays or vectors. Also, no dynamic type checking is done anywhere, this is all done at compile time.

This differentiation allows the client to directly read or write an entire byte array instead of type checking writing each value separately.

If by client you mean the device (i.e., an Arduino board), then I would expect that the compiler will optimise all of the recursive calls to the inline and templated rpcRead and rpcWrite functions away. Have you run some benchmarks to quantify the performance gain?

A useful type of array for streaming data from a device.

Useful in which way? Can you give an example where this type would be preferable over other types?

Furthermore, by briefly reading the code and running a couple of tests, I found the following issues:

• Including an entire library (PString) in this code base does not seem a good idea to me for the following reasons:
  • I foresee some licensing issues. This project is licensed under the MIT license while PString uses the GPL license.
  • The library specification support dependencies.
  • Git submodules could perhaps be used for other platforms.
• A dependency on the standard template library was added, which breaks support for AVR boards.
• The current implementation does not seem to compile for the ESP32 platform (which does support the STL).
• The current implementation does not work in the Arduino environment (tested with arduino-cli).
• Now two code styles are used in the same project.
• Documentation seems to be missing:
  • API documentation was removed, none was added.
  • There is no user documentation for the added functionality.
  • No examples are given.
• Unit tests are missing.
  • In fact, the tests no longer compile.
    

Instead of fixing these issues, I would like to suggest discussing the more general remarks above first.

[suahelen]

First of all, thanks for the consideration :) I am aware that I did not adhere to the PR standarts you require. I just thought that it is a nice feature and wanted to let you know.

Regarding:

dynamic typed arrays Sure, I do understand that this is done at compile time. I guess thats just wrong wording on my side. What I meant is the typing on the python side (which I was referring to as client). Now on the python side for every value of a list that is written or read you use a recursive loop to parse the data in to the stream. And for each of these elements you do a typecheck (again in python).

So there is no performance gain to be had on the MCU side with the implementation that I provided.

The main thing is the differentiation to use {} if the type is atomic, so that the client can optimize the write and read operation to the stream.

On the client side the performance gains are dependent on the length of the array. But for arrays around the size 1k it was close to 50x faster

The Rolling_buffer<T, s> is useful in a scenario where the Client is reading data from the MCU at a lower frequency that the data is aquired. E.q. a sensor reading at 100Hz and we read out the data at 1Hz. Its such a scenario its a nice way to have a defined size array on the MCU. So no memory issues like with a vector that could potentially grow indefinitely, should arise. Its also more convenient compared to just a normal fixed size array as it keeps track of how many values actually have been written to the buffer.

TStrings I can see why that is not a desirable thing to have an additional library included. Also I was not aware of the licensing issue. This is something that could be excluded. To be honest I should probably put that in a separate branch independent of the array stuff.

I must have made some errors in the process of moving the code into the separate repo I'll check the compile errors and fix that.

Lastly, regarding codestyle, Docs and Unit tests, I must admit I disregarded for the time beeing. As I was using these adaptations for work I was limited in the time I could Invest here. But I could imagine to fix that in some spare time.

[jfjlaros]

The main thing is the differentiation to use {} if the type is atomic, so that the client can optimize the write and read operation to the stream.

But then this optimisation could be implemented client side only. If the type signature does not contain any nesting (e.g., like [[h]]), then the type is atomic. Perhaps I still misunderstand something?

Its such a scenario its a nice way to have a defined size array on the MCU. So no memory issues like with a vector that could potentially grow indefinitely, should arise.

I can see how that could be useful. This data structure seems to be serialised as a regular array, so no changes on the client are needed in principle. Is this correct?

[suahelen]

But then this optimisation could be implemented client side only. If the type signature does not contain any nesting (e.g., like [[h]]), then the type is atomic. Perhaps I still misunderstand something?

Hmm, I think you are correct there. I'll check this out tomorrow.

I can see how that could be useful. This data structure seems to be serialised as a regular array, so no changes on the client are needed in principle. Is this correct?

Correct

[suahelen]

I have fixed the build errors for now. If I get the oprimized array read to run with only client updates I'll let you know and close the Pull request. And add the changes in the client repo. In case you are still interested I can open a new one with only the Rolling buffer. Just let me know.

[suahelen]

Just tested it on the and it works. I'll update the PR on the python repo accordingly.