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

Add support of Toshiba Remote Control B #2093

Closed
wants to merge 8 commits into from
Closed

Add support of Toshiba Remote Control B #2093

wants to merge 8 commits into from

Conversation

jackysze
Copy link
Contributor

@jackysze jackysze commented May 12, 2024

Add support to Toshiba Remote Control B

The third byte in the raw state array originally represents payload length only. It was found that the first 4 bits is being used to identify whether current remote control is A or B. #2089

  1. Modify the struct to accommodate both payload length and remote control type.
  2. Add constants and functions to set/get current remote control type.
  3. Fix the getInternalStateLength function to extract the last 4-bits instead using 8-bits.

@NiKiZe
Copy link
Collaborator

NiKiZe commented May 12, 2024

Nice!

Consider adding remote model to text representation output, check other AC protocols where there is different models of remote. And also tests for this if possible?

@@ -493,6 +494,23 @@ String IRToshibaAC::toString(void) const {
return result;
}

void IRToshibaAC::setRemoteControl(const uint8_t remote_type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use model, as example see

void IRVoltas::setModel(const voltas_ac_remote_model_t model) {

@@ -490,9 +491,37 @@ String IRToshibaAC::toString(void) const {
result += addBoolToString(getEcono(), kEconoStr);
result += addBoolToString(getFilter(), kFilterStr);
}
switch (getRemoteControl()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is addModelString helpers

@NiKiZe
Copy link
Collaborator

NiKiZe commented May 13, 2024

Nice!

Consider adding remote model to text representation output, check other AC protocols where there is different models of remote. And also tests for this if possible?

As mentioned here, please use the existing concept of remote model, this goes for all of the namings, take inspiration of the example linked inline, as well as other protocols using remote models, try to reuse as much as possible of the existing framework, strings etc.

@jackysze jackysze closed this May 13, 2024
@NiKiZe NiKiZe reopened this May 13, 2024
@jackysze jackysze closed this May 13, 2024
@NiKiZe
Copy link
Collaborator

NiKiZe commented May 13, 2024

You should keep using this branch and PR

@jackysze
Copy link
Contributor Author

jackysze commented May 13, 2024

I'm sorry, but I would prefer to close my pull request as I don't have much time to investigate the entire codebase and adhere to all coding standards. However, I believe my findings could assist the main contributors in implementing this enhancement to meet the team's standards. Thank you.

@NiKiZe
Copy link
Collaborator

NiKiZe commented May 13, 2024

Thanks for posting a comment.

feel free to just leave th PR open and we can finalize it, it's a great work that just needs some final adjustments.

@jackysze
Copy link
Contributor Author

I've reverted the commit on my branch to cleanup the lousy changes with a force push and seems the PR can't be reopened.

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