-
Notifications
You must be signed in to change notification settings - Fork 833
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
Supported FUNIKI remote AC #2113
base: master
Are you sure you want to change the base?
Supported FUNIKI remote AC #2113
Conversation
090ea2e
to
efbb86b
Compare
efbb86b
to
9170586
Compare
Change argument order and remove unused parameters.
Change argument order and remove unused parameters. Fix a doxygen error Fix a typo/error in the Ptr comment.
Can anyone help me resolve this failing after check PRs below? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. Looks mostly good.
Can you supply some raw data and what the "raw" A/C message is supposed to be/do etc. i.e. the settings state.
src/ir_Funiki.h
Outdated
/// @brief Support for FUNIKI A/C protocols. | ||
/// @see https://github.com/crankyoldgit/IRremoteESP8266/issues/2112 | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the supported devices info here please. It's required for our automatic documentation.
e.g. Like the following for Kelvinator.
IRremoteESP8266/src/ir_Kelvinator.h
Lines 5 to 19 in dc0fd31
// Supports: | |
// Brand: Kelvinator, Model: YALIF Remote | |
// Brand: Kelvinator, Model: KSV26CRC A/C | |
// Brand: Kelvinator, Model: KSV26HRC A/C | |
// Brand: Kelvinator, Model: KSV35CRC A/C | |
// Brand: Kelvinator, Model: KSV35HRC A/C | |
// Brand: Kelvinator, Model: KSV53HRC A/C | |
// Brand: Kelvinator, Model: KSV62HRC A/C | |
// Brand: Kelvinator, Model: KSV70CRC A/C | |
// Brand: Kelvinator, Model: KSV70HRC A/C | |
// Brand: Kelvinator, Model: KSV80HRC A/C | |
// Brand: Gree, Model: YAPOF3 remote | |
// Brand: Gree, Model: YAP0F8 remote | |
// Brand: Sharp, Model: YB1FA remote | |
// Brand: Sharp, Model: A5VEY A/C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Let me check!
src/ir_Funiki.cpp
Outdated
/// Calculate and set the checksum values for the internal state. | ||
/// @param[in] length The size/length of the state array to fix the checksum of. | ||
void IRFunikiAC::checksum(const uint16_t length) { | ||
(void)(length); | ||
// Funiki uses the same checksum alg. as Kelvinator's block checksum. | ||
// _.Sum = IRKelvinatorAC::calcBlockChecksum(_.remote_state, length); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense. If there is no checksum, then you shouldn't need this procedure.
And the validChecksum()
function shouldn't be needed. If it is needed, the the checksum should do something.
If they are both un-needed, they should be removed.
src/ir_Funiki.cpp
Outdated
/// Set the model of the A/C to emulate. | ||
/// @param[in] model The enum of the appropriate model. | ||
void IRFunikiAC::setModel(const funiki_ac_remote_model_t model) { | ||
switch (model) { | ||
case funiki_ac_remote_model_t::UNKOWN: | ||
default: _model = funiki_ac_remote_model_t::UNKOWN; | ||
} | ||
} | ||
|
||
/// Get/Detect the model of the A/C. | ||
/// @return The enum of the compatible model. | ||
funiki_ac_remote_model_t IRFunikiAC::getModel(void) const { return _model; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it doesn't do anything. If the model functions are not needed, they should be removed.
src/ir_Funiki.cpp
Outdated
// /// Set the A/C's timer to turn off in X many minutes. | ||
// /// @param[in] minutes The number of minutes the timer should be set for. | ||
// /// @note Stores time internally in 30 min units. | ||
// /// e.g. 5 mins means 0 (& Off), 95 mins is 90 mins (& On). Max is 24 hours. | ||
// void IRFunikiAC::setTimer(const uint16_t minutes) { | ||
// uint16_t mins = std::min(kFunikiTimerMax, minutes); // Bounds check. | ||
// setTimerEnabled(mins >= 30); // Timer is enabled when >= 30 mins. | ||
// uint8_t hours = mins / 60; | ||
// // Set the half hour bit. | ||
// _.TimerHalfHr = (mins % 60) >= 30; | ||
// // Set the "tens" digit of hours. | ||
// _.TimerTensHr = hours / 10; | ||
// // Set the "units" digit of hours. | ||
// _.TimerHours = hours % 10; | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the commented out function. Try to avoid including code that doesn't work etc. Someone might think it works and try it.
src/ir_Funiki.cpp
Outdated
stdAc::state_t IRFunikiAC::toCommon(void) { | ||
stdAc::state_t result{}; | ||
result.protocol = decode_type_t::FUNIKI; | ||
result.model = _model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If model
s isn't used, return -1 (I think)
src/ir_Funiki.cpp
Outdated
// for(int i = 0; i < results->rawlen; i++) | ||
// { | ||
// printf("%d, ",results->rawbuf[i]); | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these comments please.
src/ir_Funiki.cpp
Outdated
if (strict && nbits != kFunikiBits) | ||
return false; // Not strictly a Funiki message. | ||
|
||
// printf("\nlen: %d\n", results->rawlen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented code.
src/ir_Funiki.cpp
Outdated
// // Inter-block gap + Data Block #2 (32 bits) + Footer | ||
// if (!matchGeneric(results->rawbuf + offset, results->state + 4, | ||
// results->rawlen - offset, nbits / 2, | ||
// kFunikiBitMark, kFunikiMsgSpace, | ||
// kFunikiBitMark, kFunikiOneSpace, | ||
// kFunikiBitMark, kFunikiZeroSpace, | ||
// kFunikiBitMark, kFunikiMsgSpace, true, | ||
// _tolerance, kMarkExcess, false)) return false; | ||
|
||
// // Compliance | ||
// if (strict) { | ||
// // Verify the message's checksum is correct. | ||
// if (!IRFunikiAC::validChecksum(results->state)) return false; | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented out code please.
src/ir_Funiki.cpp
Outdated
tole, kMarkExcess, false); | ||
if (used == 0) return false; | ||
offset += used; | ||
// printf("\n ooffset: %d\n", offset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented out code please.
I've given you a code review. See the review comment for the Basically you are missing some info in the header file that we scan for to help automatically build some of the documentation for the project. Namely |
Now I have just updated new commit. I removed un-needed something as you recommended. And you can also see "All checks have passed". |
This merge request adds basic support for the FUNIKI remote. This includes support for almostly decode data from remote as power on/off, temperature, mode, fan, clock, timer on/off, swingV, sleep,.. and support sending almostly setting except timer on/off.
For information about this remote, you can refer this issue at #2112
It is old remote model. Data has no checksum byte, and is very easy to decode.
Thanks!