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

IR/receiver unification #4198

Open
DedeHai opened this issue Oct 17, 2024 · 11 comments
Open

IR/receiver unification #4198

DedeHai opened this issue Oct 17, 2024 · 11 comments

Comments

@DedeHai
Copy link
Collaborator

DedeHai commented Oct 17, 2024

I am opening this issue for a discussion on how to improve / unify the (IR) remote handling.

The IR receiver currently uses around 20kB of flash which is excessive. Parts of that is coming from the special-case handling of IR commands, the other (larger) part from the bloated IR receiver library IRremoteESP8266. I think there is much room for improvement.

Here are my suggestions / questions:

  • is it possible for the currently supported remotes to also use the API format? Are there any functions that would not work if changing them all to use JSON API like for a custom remote?
  • if it is possible to use the API: where to place the json files for the remotes? I imagine one file per remote-type being uploaded to the file system (in a subfolder?) Question is how to upload the files: on a fresh install this could be done with a script but on OTA the files would be missing. Or is there an easy way to add files to the FS when doing OTA (for offline systems)?
  • Integrate the necessary code for reading IR signals directly into WLED instead of using the library, just keeping the required functionality. This could potentially also reduce ram usage for buffers.

Making all receivers use the API would also enable users to easily change IR key functionality by editing the corresponding file.

@softhack007
Copy link
Collaborator

softhack007 commented Oct 17, 2024

Hi,

I think we cannot re-implement the IR receiver code, and I'm not a fan of "slicing a library" to just use the code we want. Compilers are able to remove unused functions (dead code) when creating the binary.

However the de-facto standard library from ArminJo does support 8266 and esp32 since a long time. 🤔 Maybe it has a smaller FLASH footprint.

https://github.com/Arduino-IRremote/Arduino-IRremote

@DedeHai
Copy link
Collaborator Author

DedeHai commented Oct 17, 2024

I think we cannot re-implement the IR receiver code

Any reason for that? The behaviour would need to be identical to current, just over the API. Or is the problem providing the json files in OTA? I think "fetch them from github" is out of the question...

and I'm not a fan of "slicing a library" to just use the code we want. Compilers are able to remove unused functions (dead code) when creating the binary.

I agree, but on the other hand using a library that uses that much flash has become a bit of an issue.

However the de-facto standard library from ArminJo does support 8266 and esp32 since a long time. 🤔 Maybe it has a smaller FLASH footprint.

https://github.com/Arduino-IRremote/Arduino-IRremote

That is a good input. I can check if it can be a 1:1 replacement.

@softhack007
Copy link
Collaborator

softhack007 commented Oct 17, 2024

Any reason for that? The behaviour would need to be identical to current, just over the API. Or is the problem providing the json files in OTA? I think "fetch them from github" is out of the question

Sorry, maybe this was a misunderstanding. My statement was related to the second part, so "re-implement the receiver code" for me meant "create our own IR driver". This would be very low-level and a tricky thing to achieve.

For sure we can replace all the hard-coded commands with IR.json. People can already upload their custom IR.json from the UI. What's missing right now is a possibility to pre-populate the LittleFS partition with files during install - neither OTA nor online web install currently allow this.

@softhack007
Copy link
Collaborator

softhack007 commented Oct 17, 2024

That is a good input. I can check if it can be a 1:1 replacement.

Might not be a 1:1 replacement, because the API changed a bit in arduino-irremote version 3, while irremote8266 still implements the older "v2" api. Also the IR codes from arduino-irremote might be different because they fixed some bitorder decoding problems in v3.

@blazoncek
Copy link
Collaborator

AFAIK WLED only needs decoding IR signal. All other processing is done in WLED.

@blazoncek
Copy link
Collaborator

I am all in favour of using JSON for IR remotes. However...

Not all users are savvy enough to upload correct JSON file and many would like to use remote OOB with no intervention.
We could create JSON files on 1st boot and use those later. If the user modifies them later that could be an issue.

For me, I created an universal JSON file. It contains commands for multiple different remotes I use. Unfortunately some generic remotes re-use same key ID for different buttons, i.e. "brightness up" on one remote has the same key ID as "red" on another. So you cannot have a truly universal JSON file.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Oct 18, 2024

I was thinking about this problem a bit.
Uploading to the FS is not possible in a OTA update without wiping the file system. Having a 'generate IR files' binary as an intermediate step was another idea, but that is unacceptably user-unfriendly.
Loading the file from KB or git when a user selects the remote and saves settings would be an option but it requires internet access, so not a good option.
So generating the json file (or files, it could be multiple for the known remotes) is the only viable option I see.
OR: could the json commands for the existing remotes be generated 'on the fly' and passed as a string instead of generating the file? Not sure that will save any code but it would be more 'streamlined'.

regarding the IR library: I will look at that in the coming days to check if any other library would be better or if we should just spin our own code. Reading the signal is very easy, the library currently uses a pin interrupt and micros() to timestamp (if I interpret the code correctly) which is just a few lines of code. The magic happens in the decoding, that I did not yet look at.

BTW: in the 40key blue remote json file in the KB there is an error (or in WLED code): Won and Woff codes are defined differently in WLED code and that json file, found this by accident where I randomly picked a file and commands to look at how the json IR works. also behaviour is slightly different (lastwhite vs. full bright).

@blazoncek
Copy link
Collaborator

could the json commands for the existing remotes be generated 'on the fly' and passed as a string instead of generating the file?

That's what I'm proposing in

We could create JSON files on 1st boot and use those later

There are a few issues with this:

  • what happens if user edits such JSON file
  • if user edited JSON how to proceed if user selects another type
  • do we need pre-defined remotes in code if using JSON (i.e should we use one generic ir.json or multiple files for each remote ir18key.json, ir24key.json, ir44key.json, ircustom.json, etc)

Also, do not forget there is also ESP-NOW support for Wiz remotes (albeit flaky ATM).

@DedeHai
Copy link
Collaborator Author

DedeHai commented Oct 18, 2024

I mean explicitly NOT generating a file but more or less keep it the way it is now but instead of calling functions it generates a string with a single command and passes that to the "API interpreter". The Idea is that this "interpreter" extends the command such that the normal JSON decoder can read it. WIZmote could use that API decoder too.
Just a thought.

@blazoncek
Copy link
Collaborator

Are you talking about deserializeState() as "API interpreter"?
I do not see the benefit of that in the current implementation.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Oct 18, 2024

Yes. Plus the function that currently handles ir.json. There is no real benefit, just unification of how things work. RN there is two ways for IR, json and the local IR functions doing the same thing.
Using json 'codes' would be almost the same as generating a file: you need the same functionality to generate the file as you do to generate json commands on the fly. A ir.json could still be used. The on the tly method is a way to solve the file issues you mentioned.
Does that make sense?

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

No branches or pull requests

3 participants