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

Refactor different Daikin protocols into separate files #1970

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

Conversation

caliston
Copy link

ir_Daikin.cpp is quite unwieldy, with about 4000 LOC. It covers several different Daikin IR protocols in a single file. That makes it hard to work on adding extra protocols because there's a lot of similar-but-different code for each one (some selected by #defines, some not).

This PR refactors that code so that each DaikinNNN protocol is in a separate file. The header file of ir_Daikin.h and tests remain unified, so it should make no difference to usage of the library.

Checked linting and passes all the tests.

@caliston
Copy link
Author

Hmm, this is failing the supported-devices-check because that's expecting one header file per .cpp. It complains if the 'Supported' section is in the ir_Daikin.h header file because the new .cpps don't have a Supported section, but if I move that into the .cpp it complains it's not in the header file. I don't really want lots of ir_DaikinNNN.h because ir_Daikin.h is where most of the common code lives.

Any suggestions for the best approach? I suppose I could do something just for the purposes of passing the test, like having an ir_DaikinNNN.h just for the 'Supported' tags, but that wouldn't be very pleasant.

@crankyoldgit
Copy link
Owner

Hmm, this is failing the supported-devices-check because that's expecting one header file per .cpp. It complains if the 'Supported' section is in the ir_Daikin.h header file because the new .cpps don't have a Supported section, but if I move that into the .cpp it complains it's not in the header file. I don't really want lots of ir_DaikinNNN.h because ir_Daikin.h is where most of the common code lives.

Any suggestions for the best approach? I suppose I could do something just for the purposes of passing the test, like having an ir_DaikinNNN.h just for the 'Supported' tags, but that wouldn't be very pleasant.

Yeah, this is a hard one. I don't know the best approach either, otherwise it would have been refactored.
Perhaps the better approach is to change how the check is performed to handle this case, and to ensure the scrape_supported_devices.py works too

@caliston
Copy link
Author

Thanks, I'll take a look at that. Won't be able to do so for some weeks, but I'll put it on the todo list when I'm able to.

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.

2 participants