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

IRremoteESP8266 in a python library #2067

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

frawau
Copy link

@frawau frawau commented Feb 18, 2024

I wonder if you would accept this pull request.

It is quite a hack, I never used C++ and I am lazy so I took the path of least resistance.

What this PR does is add the ability to add IRremoteESP8266 to Python. I use it in my library pyhvac to generate HVAC IR codes with Python.

To avoid too many modifications to the original C++ code I hacked things this way:

  • Only expose IRac
  • Create a global vector
  • Modify "mark" and "space" of IRSend to concatenate the values passed to them to the global vector
  • Add 2 methods to the IRac class:
    -- getTiming to retrieve the value of the global vector
    -- resetTiming to reset the vector

All this is controlled by the preprocessor variable PYTHONLIB

I initially planned to generate Mark and Space pulses in Python (that is why the timing info is present in pyhvac) but that would have required a lot of ifdef/else in the C++ code.

I also thought about having the vector as a member of the IRac class, but this too would have required a lot of changes.

Using a global vector means that in some instances, access to the object needs to be serialized in Python, but I can live with that.

To generate the Python lib, I use SWIG and one must use the latest version (4.2.0 at the time of this writing) to generate the library.

Once the library is created you can use it like this:

import irhvac
ac=irhvac.IRac(4)
ac.next.protocol = irhvac.ELECTRA_AC
ac.next.model = irhvac.DG11J13A
ac.next.mode = irhvac.opmode_t_kCool
ac.next.degrees = 23.5
ac.next.swingv = irhvac.swingv_t_kAuto
ac.sendAc()
print(ac.getTiming())
ac.resetTiming()

Since SWIG is used, it should be quite simple to add support for other languages.

I did run the tests. They all passed on x86_64. The Lego test failed on ARM64, but this is not an A/C IR and I guess is more a mishandled endianess problem.

@NiKiZe
Copy link
Collaborator

NiKiZe commented Feb 18, 2024

Other than the linter issues. Could you add a test workflow?
I would like to think that most of the inner workings is already available as used by tests / tools, could the C changes be minimized by re-using that?

@frawau
Copy link
Author

frawau commented Feb 18, 2024

I am not sure what you mean by adding a test workflow. What do you want me to test? The aim of the PR is to not change the code unless PYTHONLIB is defined. So the one test I could think of is compiling the Python extension and do some tests to check the output is what is expected..... But that would require using swing 4.2.0.. That was released on Dec 31st 2023 and so it will require some time before it will be widely adopted.

As for minimizing code changes... I can see that somewhere in the test, you get to output a list of string containing timing information like "m520s1520..." but, as I wrote, I really don't know C++ so I could not figure how to get that. Hence my pretty minimal patch to the C++ code.

@NiKiZe
Copy link
Collaborator

NiKiZe commented Feb 18, 2024

You have for example
https://github.com/crankyoldgit/IRremoteESP8266/blob/master/tools/code_to_raw.cpp

You have python tests in this PR?
How would we make those tests run automatically in GitHub actions? If this requires swing 4.2.0 then have that workflow but keep it disabled until available.

@NiKiZe
Copy link
Collaborator

NiKiZe commented Feb 18, 2024

Several tests fail to build with

/home/runner/work/IRremoteESP8266/IRremoteESP8266/src/IRsend.h:259:3: error: 'VIRTUALMS' does not 
 name a type; did you mean 'VIRTUAL'?
   VIRTUALMS void space(uint32_t usec);
   ^~~~~~~~~
   VIRTUAL

It is not clear to me what this is intended to do?

@frawau
Copy link
Author

frawau commented Feb 18, 2024

Several tests fail to build with

/home/runner/work/IRremoteESP8266/IRremoteESP8266/src/IRsend.h:259:3: error: 'VIRTUALMS' does not 
 name a type; did you mean 'VIRTUAL'?
   VIRTUALMS void space(uint32_t usec);
   ^~~~~~~~~
   VIRTUAL

It is not clear to me what this is intended to do?

From what I understood, "VIRTUAL" is defined as "virtual" when TEST_UNIT is defined, the would lead the preprocessor to generate

    virtual  void space(uint32_t usec);

When TEST_UNIT is defined and

    void space(uint32_t usec);

When it is not defined. My poor understanding of C++ lead me to believe that a method defined with "virtual" exists but does nothing.

In this particular instance, I wanted the behaviour of UNIT_TEST and PYTHONLIB to diverge and have "mark" and "space" to actually have an effect when PYTHONLIB is defined. So I split the definition of VIRTUAL into VIRTUAL and VIRTUALMS (MS for Mark Space) to effect that. But this is a the preprocessor level, I am not sure why that would affect anything.

To be sure, I just ran

 make run

In the test directory and it all went peachy. I attach the output of that run.

test_run.txt

@frawau
Copy link
Author

frawau commented Feb 18, 2024

Yes... I see the issue. But it is not a test is it?

@frawau
Copy link
Author

frawau commented Feb 18, 2024

OK, not a test in the test directory. ;)

@frawau
Copy link
Author

frawau commented Feb 18, 2024

As for Python tests in this project.

No I did not add any Python test here. I do feel that it is not the place for it. Having the ability to create a Python library is IMO a great thing, but it should not distract from the fact that this project is all about generating IR codes on an Arduino platform, failure to build a Python library should not be an impediment to move forward with the Arduino bits.

I believe that pyhvac (or any other tool/library that would take advantage of this) would be a better place for those tests (Not there yet) with an automated creation of Python wheels with cibuildwheel upon new releases....

But this is your project, tell me what you think.

@NiKiZe
Copy link
Collaborator

NiKiZe commented Feb 18, 2024

I think that any "feature" that is added that then "needs to be maintained" should have enough tests to know that any changes don't break that feature, or rather at least that we know that that feature breaks.

In this case that it it builds and a minimal check of the interface.

In the bigger picture, I think we should have TOOL/LIBRARY build define that could be reused. And avoid any "python specific" ifdefs.
Will check to see if I get some time this week to maybe make a suggestion on that path.

@frawau
Copy link
Author

frawau commented Feb 19, 2024

OK, I will try to find a way to automatically check we can run swig and compile the "library".

If you don't like PYTHONLIB we could use SWIG or SWIGLIB

@frawau
Copy link
Author

frawau commented Feb 21, 2024

Just to be sure you noticed, I added an action that will:

  • download swig-4.2.0
  • compile it
  • use the just built swig to create the needed interface
  • build Python library
  • run a simple test that the library can generate the correct timing list.

…thon specifics since one could use swig create libraries for other languages.
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

2 participants