Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setting for IRremoteESP8266 tolerance #14555

Merged
merged 2 commits into from
Jan 22, 2022

Conversation

hr-kapanakov
Copy link

Description:

Add setting that allow changing IRremoteESP8266 tolerance.
Discussion as ref: #14435

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works with Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works with Tasmota core ESP32 V.2.0.2.1
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@s-hadinger
Copy link
Collaborator

You beat me on this one. It was on my todo list, well done. Theo was also proposing to reuse P_ex_TUYA_VOLTAGE_ID as you did.

I'm just worried about leftovers in P_ex_TUYA_VOLTAGE_ID. Also if a user updates with OTA, the value of the option will be zero which translates to zero tolerance; and things will break. I.e. the options equals zero should not have any regression.

Is Tolerance==0 even valid?

@hr-kapanakov
Copy link
Author

Yes, I am agree about the leftovers. About zero tolerance it seems ok based on the IRremoteESP8266 docs - https://crankyoldgit.github.io/IRremoteESP8266/doxygen/html/classIRrecv.html#aa091c449db70c65fd0221669df7438ea. I also checked the code and also seems fine to be zero: https://github.com/crankyoldgit/IRremoteESP8266/blob/93af543dcb849918b0c342ed1b6af68953a61d06/src/IRrecv.cpp#L1123
The only problem is that in such case the IR signal should match at 100% which most probably is not possible in real world.

@s-hadinger
Copy link
Collaborator

I propose to 'sacrifice' the zero value, which does not really makes sense. You can still set tolerance as low as 1%.

If tolerance equals zero, then set it to IR_RCV_TOLERANCE

Does it make sense?

@hr-kapanakov
Copy link
Author

Yeah, it seems a good compromise. I'll make the change.

@arendst
Copy link
Owner

arendst commented Jan 22, 2022

Thx. I need to make one final chk if ex tuya voltage id isn't used for version delta.

once chkd today i will merge.

@arendst arendst merged commit d625e7b into arendst:development Jan 22, 2022
arendst added a commit that referenced this pull request Jan 22, 2022
Add command ``SetOption44 1..100`` to set base tolerance percentage for matching incoming IR messages (default 25, max 100) (#14555)
@sfromis
Copy link
Contributor

sfromis commented Feb 7, 2022

Looks like the "sacrifice" of the zero value to mean default of 25 did not work for existing configs. When upgrading firmware on an IR puck, it stopped working, with SetOption44 being zero. Only after manually setting the tolerance, I could use a remote control again.

While zero for default works when issuing the command SetOption44 0, it becomes a breaking change for existing configs, where the existing value of the config option byte will usually be zero.

@s-hadinger
Copy link
Collaborator

@sfromis You are right, IrReceiveUpdateTolerance() is never called when doing an update. I suggest it is called in IrReceiveInit() too

@s-hadinger s-hadinger mentioned this pull request Feb 7, 2022
6 tasks
@s-hadinger
Copy link
Collaborator

@sfromis It should be fixed now

@sfromis
Copy link
Contributor

sfromis commented Feb 7, 2022

Works now, thanks

Tested by reverting to a binary from 2022-01-18, SetOption44 0, verified that it worked, upgraded to latest precompiled dev binary, saw SetOption44 being 25 right after the restart, and the remote worked as with the older binary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants