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

Supported FUNIKI remote AC #2113

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

Conversation

QuangThai2297
Copy link

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!

@QuangThai2297 QuangThai2297 changed the title supported FUNIKI remote AC Supported FUNIKI remote AC Jun 29, 2024
@QuangThai2297 QuangThai2297 force-pushed the feature/funiki-85bits-remote branch 7 times, most recently from 090ea2e to efbb86b Compare June 29, 2024 08:46
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.
@crankyoldgit crankyoldgit self-assigned this Jun 29, 2024
@crankyoldgit crankyoldgit self-requested a review June 29, 2024 09:13
@QuangThai2297
Copy link
Author

QuangThai2297 commented Jun 29, 2024

Can anyone help me resolve this failing after check PRs below?
"Library Linter / supported-devices-check (pull_request) Failing after 6s"
I am new here and this is the first time I contribute this project! Thanks!

Copy link
Owner

@crankyoldgit crankyoldgit left a 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


Copy link
Owner

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.

// 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

Copy link
Author

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!

Comment on lines 126 to 132
/// 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);
}
Copy link
Owner

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.

Comment on lines 144 to 155
/// 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; }
Copy link
Owner

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.

Comment on lines 339 to 353
// /// 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;
// }
Copy link
Owner

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.

stdAc::state_t IRFunikiAC::toCommon(void) {
stdAc::state_t result{};
result.protocol = decode_type_t::FUNIKI;
result.model = _model;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If models isn't used, return -1 (I think)

Comment on lines 521 to 524
// for(int i = 0; i < results->rawlen; i++)
// {
// printf("%d, ",results->rawbuf[i]);
// }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these comments please.

if (strict && nbits != kFunikiBits)
return false; // Not strictly a Funiki message.

// printf("\nlen: %d\n", results->rawlen);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented code.

Comment on lines 557 to 570
// // 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;
// }
Copy link
Owner

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.

tole, kMarkExcess, false);
if (used == 0) return false;
offset += used;
// printf("\n ooffset: %d\n", offset);
Copy link
Owner

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 Show resolved Hide resolved
@crankyoldgit
Copy link
Owner

Can anyone help me resolve this failing after check PRs below? "Library Linter / supported-devices-check (pull_request) Failing after 6s" I am new here and this is the first time I contribute this project! Thanks!

I've given you a code review. See the review comment for the ir_Funiki.h file.

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 https://github.com/crankyoldgit/IRremoteESP8266/blob/master/SupportedProtocols.md

@QuangThai2297
Copy link
Author

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.

Now I have just updated new commit. I removed un-needed something as you recommended. And you can also see "All checks have passed".
For raw message from remote capture, you can refer attached file below:
FUNIKI_remote_AC_Raw_code_decode.txt

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

Successfully merging this pull request may close these issues.

2 participants