search

mitch7391 / homebridge-cmd4-AdvantageAir

Catered shell script to integrate air conditioner control units by Advantage Air into HomeKit using the plug-in homebridge-cmd4.
MIT License
38 stars 5 forks source link

[Pull Request] Allow single quotes on accessory names and performance enhancement to lights/things devices #90

Closed uswong closed 6 months ago

uswong commented 7 months ago

name: Pull Request about: Resolve an issue or add an improvement to homebridge-cmd4-AdvantageAir. title: "Allow single quotes on accessory names and performance enhancement to lights/things devices" labels: pull-request assignees: mitch7391

With the recent release of Cmd4 v7.0.2, it resolved the issue of mutilated accessory names. AdvAir.sh can now take advantage of that enhancement to also allow lights and things devices to have their accessory names with single quotes. Moreover, the performance for lights and things devices can be further improved by using their ID directly rather than needing to parse for their IDs using their accessory names on the fly. Additionally, some improvement is also done to the "myZone switch" for systems with temperature sensors.

Is your pull request related to a problem or a new feature? Please describe:

1) Bug A lights or things accessory names with single quotes passed to the ADVAir.sh script via State_cmd_suffix are mutilated. 2) Enhancements (a) The lights or things devices require their IDs to operate and they are parsed on the fly from their names thus slow down the operation. (b) Since IOS 17, the SwingMode characteristic, which was repurposed to be myZone switch, has disappeared from Homekit primary page but hidden deep inside the Homekit Setting. It has become very cumbersome to locate making it not very useful.

Describe the solution you'd have implemented:

1) The displayName's single-quotes bug was resolved with the release of Cmd4 v7.0.2. Within AdvAir.sh, the accessory name for lights and things can now be taken directly from the dispayName, hence resolving the issue of mutilated name parsed from the state_cmd_suffix parameters. 2) Use the ID rather than the name of the lights and things as a parameter in stae_cmd_suffix during the ConfigCreator process, hence parsing of the ID on the fly using the accessory name within AdvAir.sh is no longer required. This will improve the performance of the plugin. 3) Use RotationDirection characteristic rather than SwingMode as a proxy for myZone switch. RotationDirection characteristic will show as a round button switch on the Homekit primary page. 4) Added unit tests to test for lights and things ID as a parameter in state_cmd_suffix and also added unit tests to test for RotationDirection characteristics.

  1. Updated README to reflect the changes to point 3.

Do your changes pass local testing:

Additional context:

This update is backward compatible. If you update to this latest version but do not update the config, the plugin will work just fine but you will not benefit from improvement done on this update.

To benefit from this update, install Cmd4 v7.0.2 and rerun the updated ConfigCreator, delete the homebridge cache and restart Homebridge. It is important that you have to delete the homebridge cache before restarting homebridge.

8lackIce commented 7 months ago

I’ve just uninstalled the homebridge-cmd4-AdvantageAir plugin, twice, because it did not work, at all. The plugin is however, still visible in the plugins view.

I’m not going to try to uninstall it again.

I have no idea how you guys have this thing working, but it’s a massive fail for me.

On Tue, 26 Dec 2023 at 8:55 pm, Ung Sing Wong @.***> wrote:

name: Pull Request about: Resolve an issue or add an improvement to homebridge-cmd4-AdvantageAir. title: "Allow single quotes on accessory names and performance enhancement to lights/things devices" labels: pull-request assignees: mitch7391

With the recent release of Cmd4 v7.0.2, it resolved the issue of mutilated accessory names. AdvAir.sh can now take advantage of that improvement to also allow lights and things devices to have their accessory names with single quotes. The performance for lights and things devices can be further improved by using their ID directly rather than needing to parse for their ID using their accessory names on the fly. Improvement is also done to myZone switch for systems with temperature sensors.

Is your pull request related to a problem or a new feature? Please describe:

  1. Bug A lights or things accessory names with single-quotes currently passed to the script via State_cmd_suffix will be mutilated.
  2. Enhancements (a) The lights or things devices require their IDs to operate and they are currently parsed on the fly from their names thus slow down the operation. (b) Since IOS 17, the SwingMode characteristic has disappeared from Homekit primary page but hidden deep inside the Homekit Setting where users find it very cumbersome to locate.

Describe the solution you'd have implemented:

  1. The displayName's single-quotes bug was corrected with the release of Cmd4 v7.0.2. Within AdvAir.sh, the accessory name for lights and things can now be taken directly from the dispayName, hence resolving the issue of mutilated name parsed from the state_cmd_suffix parameters.

  2. Use the ID rather than the name of the lights and things as a parameter in stae_cmd_suffix during the ConfigCreator process, hence parsing of the ID on the fly using the accessory name within AdvAir.sh is no longer required. This will improve the performance of the plugin.

  3. Use RotationDirection characteristic rather than SwingMode as a proxy for myZone switch. RotationDirection characteristic has a button switch on the primary page.

  4. Added unit tests to test for lights and things ID as a parameter in state_cmd_suffix and also added unit tests to test for RotationDirection characteristics.

  5. Updated README to reflect the changes on point 3.

Do your changes pass local testing:

  • Yes
  • No

Additional context:

This update is backward compatible. If you update to this latest version but do not update the config, the plugin will work just fine but you will not benefit from improvement done on this update.

To benefit from this update, install Cmd4 v7.0.2 and rerun the updated ConfigCreator, delete the homebridge cache and restart Homebridge. It is important that you have to delete the homebridge cache before restarting homebridge.

You can view, comment on, or merge this pull request online at:

https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/90 Commit Summary

File Changes

(19 files https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/90/files)

Patch Links:

- https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/90.patch

https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/90.diff

— Reply to this email directly, view it on GitHub https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/90, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIM5SLLFIYK34NQYSM55733YLKNK3AVCNFSM6AAAAABBDDTPJGVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA2TMMRUG44TMNQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

mitch7391 commented 7 months ago

@8lackIce I believe the changes you have attempted require an update of homebridge-cmd4 to utilise the new functionality of using the ’ characteristic in this plug-in’s config; please make sure you have updated that plugin to take advantage of the changes if you have not already.

If it is in regards to using it in accessory names for MyPlace ‘Lights’ and ‘Things’ as mentioned in this pull request, this is the submission of a proposed change I have not yet had a chance to examine or roll out to users yet.

If you are ever getting yourself to the point of frustration please feel free to reach me on mitchwilliams7391@hotmail.com before you get to that point and I am sure I can help you find a solution; we have very very rarely ever rolled out a version that has broken the experience for users.

mitch7391 commented 7 months ago

Whoops didn’t mean to close that, stupid mobile UI…

mitch7391 commented 7 months ago

@uswong please correct me if I wrong about the use of ’ prior to the roll out of this pull request.

uswong commented 7 months ago

Hey Mitch, depending on what system user has, if the system only has aircon system (e.g. E-zone) and with ' in some of accessory names, then all they have to do is to install Cmd4 v7.0.2 to get them working without needing this PR update.

If users have myPlace system which includes lights or things devices and have ' in the lights or things accessories' names, then they need this pull request to get it all working for them. If they want to get them working now, then they will have to remove the ' in their accessory names.

ztalbot2000 commented 7 months ago

Hi guys,

If you create an update to AdvAir that has a requirement in your package.json to Cmd4 v7.0.2, your users will get everything fixed at once.

Ttyl, John

On Wed, Dec 27, 2023 at 9:33 AM Ung Sing Wong @.***> wrote:

Hey Mitch, depending on what system user has, if the system only has aircon system (e.g. E-zone) and with ' in some of accessory names, then all they have to do is to install Cmd4 v7.0.2 to get them working without needing this PR update.

If users have myPlace system which includes lights or things devices and have ' in the lights or things accessories' names, then they need this pull request to get it all working for them. If they want to get them working now, then they will have to remove the ' in their accessory names.

— Reply to this email directly, view it on GitHub https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/90#issuecomment-1870360468, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSBCXYH46H2UF43S2S5F3LYLQWTBAVCNFSM6AAAAABBDDTPJGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZQGM3DANBWHA . You are receiving this because you are subscribed to this thread.Message ID: @.*** com>

mitch7391 commented 7 months ago

Thanks for confirming this @uswong, sounds like I had the right of it :)

If you create an update to AdvAir that has a requirement in your package.json to Cmd4 v7.0.2, your users will get everything fixed at once.

@ztalbot2000 my understanding of doing this is that when the user installs our plug-in, it also installs the latest version of homebridge-cmd4 at the same time?

uswong commented 7 months ago

I think @8lackIce has a completely different issue than the ' thing. This is because even if there are accessory names with ', the whole cmd4-advantageair plugin should run just fine (without needing to instal Cmd4 v7.0.2 and this PR update) but with those accessories which has ' as part of their names showing unresponsive on Homekit.

I suspect @8lackIce has fundamental installation issue.