search

puuu / ESPiLight

ESPiLight - pilight 433.92 MHz protocols library for Arduino
GNU General Public License v3.0
110 stars 41 forks source link

Add repeats to string parsing, fix string error checking #49

Closed melyux closed 3 years ago

melyux commented 4 years ago

This adds support for specifying repeats in the pulse train string input to stringToPulseTrain, as supported by Pilight (e.g. r:12) here and here. This resolves #48.

It also fixes an existing bug with verifying the string for the c: and p: components which always produced false negatives.

I'm not sure about the argument passing and returning implementation I've used here, so I'm open to suggestions on how to do it better. So I've held off on modifying the following test and example that use it:

https://github.com/puuu/ESPiLight/blob/969ab14113f19859e43ce7bc4db69104b30833db/tests/test_parse/test_parse.ino#L75

https://github.com/puuu/ESPiLight/blob/969ab14113f19859e43ce7bc4db69104b30833db/examples/Transmit_Raw/Transmit_Raw.ino#L20-L23

puuu commented 4 years ago

@melyux thank you for contributing!

The bug in verifying the string for the c: and p: is a good catch. I would prefer this as a separate commit (even better as a separate pull request).

About #48, I definitively welcome this feature!

But I don't like the proposal with a pointer as a function parameter. In this library I've avoided pointer parameters (unless they're an array) entirely. Furthermore, this change breaks the API, as you pointed out by changing the tests and examples.

I am open to other suggestions. Right now I don't have a good idea how to implement this in a clean way. How about a separate function int ESPiLight::stringToRepeats(const String &data, size_t maxlength) that only parse for the repeats?

melyux commented 4 years ago

Sorry about the delay. That sounds like a good solution to the problem. Another one could be returning a struct, with one element being the int length and the other being the int repeats.

melyux commented 4 years ago

Split the change into two PRs.

puuu commented 3 years ago

Sorry for my late reply.

Probably my mistake, the maxlength parameter is not needed, can you remove it?

If possible, also add a test into tests/test_parse/test_parse.ino.

melyux commented 3 years ago

Removed the maxLength parameter. Must have slipped my mind. Working on the test.

melyux commented 3 years ago

Added the test to the file mentioned.

melyux commented 3 years ago

Fixed the line length to make Travis happy.

puuu commented 3 years ago

Thanks, perfect.