search

hencou / esphome_components

Collection of own ESPhome components
25 stars 15 forks source link

Give more time to each config to listen to the remotes #35

Closed szupi-ipuzs closed 6 months ago

szupi-ipuzs commented 6 months ago

This gives more time (ie. more loops) for each radio configuration. Previously the configurations were switched in each loop iteration which was so fast that almost no packet could go through or packets were lost when resetting fifos during reconfig.

This seems to fix #34

The value LOOPS_PER_CONFIG=5 was chosen somewhat arbitrary. It works fine with my setup (nodemcuv2 + NRF24) and my two configs (see #34 for details), but I don't know if this will be ok for other users. With maximum configurations enabled this might turn out to be too long in total. So maybe this could be a parameter that settable via yaml?

hencou commented 6 months ago

Hi,

Thank you for putting me in the right direction!

In the original Sidoh's code, there is a variable "settings.listenRepeats" which is used for the same behaviour you wants to achive, but its not used in this ESPhome's component. I have added this piece of code now, tested, and it seems working here.

When this works, it means we can keep the code more aligned with Sidoh's original one, which will be my preferable way.

szupi-ipuzs commented 6 months ago

Good to know. Let me know if you need me to tests your changes!

hencou commented 6 months ago

Good to know. Let me know if you need me to tests your changes!

Yes, please, you can test it by retrieving the new code from the repo, I already submitted it there. Thank you in advance! The default value is "20", maybe you have to change it to "5" as you mentoined.

Looking forward to your results!

szupi-ipuzs commented 6 months ago

Hmm, sorry, but this doesn't work for me. Maybe the previous checks in handleListen() should also happen in this new loop?

Btw. your new inner loop re-defines and shadows the "i" variable. This should be fine in this case, but is kind of risky (cppcheck warns about this).

Anyway, I think my solution is better for esphome, as it does not block for additional time, while the new loop does.

szupi-ipuzs commented 6 months ago

Here's my counterproposal. This works fine for me.

I understand you'd prefer to keep the code as close to sidoh's as possible, but I think this is where the difference needs to be, for esphome sake. Your inner loop would extend the processing time, which is already too long - esphome already complains about this even without this inner loop:

[23:45:24][W][component:214]: Component mi took a long time for an operation (0.07 s).
[23:45:24][W][component:215]: Components should block for at most 20-30ms.
hencou commented 6 months ago

Seems a good solutions, thank you!