Api changing cleanup

[janLo]

This contains all the changes that might affect the external API. The only real impact in the tests was seen in the pulse-train-callback. Users of this feature have to change the signature that the length of the train is an uint8_t which should have no impact at all, as this was always an uint8_t that got casted to an int somewhere.

This is part of the effort to split #14 and is based on #18 .

[puuu]

This will be done after #16 and #20 .

[puuu]

I like to avoid type casting, as it makes the code easier to read. If automatic casting is not clear, we should use casting. So if you want to add casting please show a best practice or a compiler warning. Also the casting style should be consistent over the project. Now we have static_cast<uint8_t> and (unsigned) mixed, what is best practice for C++?

[puuu]

Well we can consider to modify the public api, because it is mainly historically defined. For this, we need to define rules about the public api before hacking on it. So it should be future proved and reflect best practices.

As ESPiLight is a Adruino library, it is written in C++ and should prefer C++ types over C, e.g., using String instead of char[]. Also consider the Arduino Style Guide for Writing Libraries

The current types are reflecting the original pilight types. One exception is uint16_t *pulses, which was selected to reduce memory consumption. See also, commit 8356b014abb59b289f369a6c08a618f80c8ec494 and 70b5cd199ea9eebe7b5446e4d681a42f72b18c65 . As you see, uint8_t rawlen is ESPiLight specific and might be changed in the future, depending on pilight protocol requirements.

So, for mingaplen and maxgaplen, uint16_t is not pilight the default, but should be fine and makes it more consistent.

For length, pilight default is int rawlen and the public api is reflecting this, however internaly we using uint8_t for protocol_t.rawlen and PulseTrain_t.length. Changing the api to uint8_t is not future proofed, and unsigned int can be bigger then the pilight default, whereas int8_t would be to small.

The functions sendPulseTrain(), pulseTrainToString() and stringToPulseTrain() can be changed to unsigned int as they only loop around length/maxlength.

The functions stringToPulseTrain(), createPulseTrain() and send() returning negative error values and should stick to int.

Whereas, PulseTrainCallBack, parsePulseTrain(), receivePulseTrain(), nextPulseTrainLength() minrawlen/maxrawlen are related to protocol_t.rawlen and PulseTrain_t.length.

Considering int and not negative values we have to look at:

• return of ESPiLight::parsePulseTrain() (number of matches)
• repeats in send(), sendPulseTrain() and ESPiLightCallBack

So, if we change the public api and especially the callback functions (because this hurt the users), let's do it once and right.

Personal I don't see any hurry in this, but looking forward in a fruitful discussion.

[puuu]

Btw., converting #define to const should also be considered, as it is more C++ like.

[puuu]

As for the commits:

• the "boolean" commit is merged in e57283e810f09e09f2ca0f54bc9ea56a5d89427d
• the typecasting commits, as necessary should go in a separate pull request (please consider naming of commits, e.g., filename)
• the rest is free for discussion (@janLo please rebase agains master, to get rid of unimportant commits)

[janLo]

As String from Arduino is basically a simple wrapper for a char[] I think we should always cinsider if its woth the overhead to convert the types, because the conversion between String and char[] always employs a memory allocation and a copy of the string contents. We should not forget, that the ESP is still a very limited platform.

So everywhere where are a lot of string data is exchanged I would be carefully using string to char conversion (and back). In the end, ESPiLight is just a little wrapper to make the usage of the pilight protocols easier and handle the rf-data and pilight is C and therefore uses char[] for representing string data.

For the rawlen you could argue that there might be a need in the future for more than uint8_t. But then I would go for an unsigned int or better a size_t. For input-lenghth-parameters I would definitely not use any wider types than the internal API can handle. This should also be no problem to extend at a later time. But providing a parse function that takes an unsigned int as parameter and can actually only handle an uint8_t is a reciepe for desaster.

[janLo]

Rebased & size_t

[puuu]

Using String is easier to read/understand and less error-prone than char[]. This should be enough reason to us it in external Arduino library api. For internal purpose, we can consider char[].

On the ESP platform, unsigned int or size_t is 32 bit long. Now we have 51 protocols and MAX_PULSE_TYPES is set to 16. Thus, if protocol_t.rawlen and PulseTrain_t.length is uint8_t we are allocating 67 byte, with size_t it would be 268 byte (not counted other related changes in 70b5cd199ea9eebe7b5446e4d681a42f72b18c65 ).

By the way, the highest RAW_LENGTH value of all protocols is 151 (quigg_gt1000.c), so uint8_t might be enough for a long time.

[janLo]

I changed it to size_t because I thought you said that you found uint8_t not future proof. I was fine with 8 bit.

Was there a misunderstanding?

[puuu]

Was there a misunderstanding?

From your previous comment, it sounds like you want to change rawlen to size_t. Sorry, my comment was not based on your commits.

[puuu]

@janLo please have a carefull look at the apichanges branch. I did some cleanup of your commits (rebase, reorder, changing commit messages and splitting of some commits). Also some code changes, because of compiler warnings.

Still missing, also from your previous commits:

1. ESPiLight::sendPulseTrain() Should we change length from int to size_t?

2. ESPiLight::sendPulseTrain(), ESPiLight::send(), ESPiLightCallBack Is int for repeats ok?

[puuu]

@janLo many thank! Merged with some more clean ups from 025cbfef402be5c93c37130385f053574c541e34 to 653edb1832688020be1f2234fb76ec0f26f0ee0d .