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 4 forks source link

[Pull Request] Code fix, bug fix, add an optional new feature and an optional enhancement. #85

Closed uswong closed 1 year ago

uswong commented 1 year ago

name: Pull Request about: Resolve an issue or add an improvement to homebridge-cmd4-AdvantageAir. title: "[Pull Request]" labels: pull-request assignees: mitch7391


Fixed some scripts to make it more stable and fixed a bug in AdvAir.sh and added an optional new feature and an enhancement to homebridge-cmd4-advantagaair.

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

  1. A venerability was found in the codes especially in ConfigCreator.sh. It will fail if an optional retries parameter is added in the config.json.
  2. A bug was found in AdvAir.sh such that when the aircon system is offline, the plugin still registers it as online.
  3. Added three additional Zone Control options.
  4. Added an optional performance enhancement by doing the necessary rounding in Cmd4 module.
  5. Some rebrand of AA system has used other PORT number other than 2025. For example, the Fujitsu AnywAiR, which is a rebrand of an AA system has used PORT 10211 instead of 2025. The current version of the plugin has PORT=2025 hard wired.

Describe the solution you'd have implemented:

  1. Change the coding method by using the jq routine.
  2. If the system is offline, the myAirData.txt will be set to an empty json, which will only be re-populated when the system is back online.
  3. Created three additional Zone Control options using Switch, Lightbulb, Fan or Fanv2 accessories with integration with the temperatureSensor and myZone switch wherever possible. The additional Fanv2 characteristics Active and SwingMode (used as proxy as myZone switch) are also coded in AdvAir.sh to facilitate the additional options.
  4. Added an AA specific section within the Cmd4PriorityPollingQueue.js module to do the necessary % rounding. This include the rounding of the % zone open which is in 5% step, and the rounding of aircon speed to the nearest 25%, 50% or 90% for low, mid or high fan speed. Unpermitted Set requests will also be denied at Cmd4 level before sending it to AdvAir.sh.
  5. Added an extra user input parameter for "PORT used" in config.schema.json and modified the code in index.htm, server.js, ConfigCreator.sh and AdvAir.sh to accommodate.
  6. Updated the Web UI to include the additional options of using the Switch, Lightbulb, Fan and Fanv2 accessories as proxy for Zone Controls.
  7. Added unit tests for the additional characteristics Active and SwingMode.
  8. Updated the README to include a description of the ConfigCreator optional features (item 10) and a brief instruction on how to take advantage of the performance enhancement (item 12).

Do your changes pass local testing:

Additional context:

Below is the updated Web UI for ConfigCreator with the optional features and an extra input parameter PORT used:

AdvAirConfigCreator2

How to take advantage of the performance enhancement: The enhanced version of Cmd4PriorityPollingQueue.js is included in this plugin as Cmd4PriorityPollingQueue.txt and user can optionally choose to use it by copying this Cmd4PriorityPollingQueue.txt to homebridge-cmd4 plugin directory as Cmd4PriorityPollingQueue.js overwriting the existing one. This can be achieved by running a script created after running the Web UI version of ConfigCreator.sh. This script is called copyEnhancedCmd4PriorityPollingQueueJs.sh and is located in the homebridge storage directory.

If the user is running the ConfigCreator.sh from a terminal, this enhanced version of Cmd4PriorityPollingQueue.js is copied to homebridge-cmd4 by default.

Please note that this performance enhancement is only available for users who has Cmd4 v7.0.0-beta2 or v7.0.1 installed.

To remove this enhancement, simply re-install Cmd4 plugin.

mitch7391 commented 1 year ago

Thanks for submitting this @uswong, I have already had it running for some time now and appears to be working as we want it to be :) especially the 'no response' work and the 'lightbulb' zone options. I will try find some time of the next few days to review this and get to the admin side of the work.

One thing I did notice when testing the 'lightbulb' config changes is that it changes the name from 'Living Zone' as a switch to 'Living Zone-T' as a lightbulb. I am wondering what the reasoning behind this might be?

uswong commented 1 year ago

One thing I did notice when testing the 'lightbulb' config changes is that it changes the name from 'Living Zone' as a switch to 'Living Zone-T' as a lightbulb. I am wondering what the reasoning behind this might be?

Hey Mitch,

I forgot to explain in my note that the Zone-T is meant to distinguish between those zones with temperature sensors from those zones without temperature sensors, especially for those users whose systems have mixed zones.

I did test it with Siri by just saying Living Zone without verbalizing the T for Living Zone-T, Siri still recognizes the correct zone.

Do you have other suggestions to identify between Zones with and without temperature sensors?

mitch7391 commented 1 year ago

I am wondering if need to differentiate at this level that the zone has a temperature sensor, I am hoping most users would know which zones they have do have temperature sensors.

I did test it with Siri by just saying Living Zone without verbalizing the T for Living Zone-T, Siri still recognizes the correct zone.

My biggest worry was Siri or the user feeling they have to speak the full name; some people might not be aware they can at times get away with only part of the name. I am not sure if we should rely on that exploit though, I have seen these ones and naming of devices be very picky iOS update to iOS update; and have had to change the names of some devices in my house to get better use of them via Siri.

That being said, I do not have a better solution to offer off the top oft head and will need to think about it a bit more… Not that I have touched the project in a long while, but with our standalone project I had managed to link the temperature sensors to the zone switches; so maybe this is something we can think about too…

uswong commented 1 year ago

Hey Mitch,

I have a go with it as regards to #86. It was quite easy to implement. I have now uploaded my edits to this PR. Please gitclone/install from my fork branch to test it out. It works fine in my testing. All unit tests still passed without any issue.

uswong commented 1 year ago

I am wondering if need to differentiate at this level that the zone has a temperature sensor, I am hoping most users would know which zones they have do have temperature sensors.

No worries Mitch, you do have a point on Siri too. I do think most users either have all zones are with temperature sensors or all zones are without temperature sensors. I am happy to remove it.

The only minor drawback is that if the user does change the % zone open on those zones with temperature sensor, Cmd4 will send the Set request and will have to depend on AdvAir.sh to reject it. That was the motivation for me to give it a "code" (Zone-T instead of Zone), so that Cmd4 can identify it and not send any Set request at all for % zone open for zones with temperature sensors.

I had managed to link the temperature sensors to the zone switches

That is actually a very good idea! How did you do it? I wonder we can use the Cmd4 linkTypes to achieve what you have achieved.

uswong commented 1 year ago

I do think most users either have all zones are with temperature sensors or all zones are without temperature sensors. I am happy to remove it.

Done.

I had managed to link the temperature sensors to the zone switches I wonder we can use the Cmd4 linkedTypes to achieve what you have achieved.

I did a quick test using Cmd4 linkedTypes with the AirConServer and it works! On the Switch or Lightbulb accessory, there is a display of temperature on top. This is certainly a better way to clearly identify which zone has temperature sensors and which zone does not. Shall we implement that? It is not hard to do at all. The only modification is in ConfigCreator.sh.

WhatsApp Image 2023-07-17 at 22 21 24

mitch7391 commented 1 year ago

I have a go with it as regards to https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/issues/86. It was quite easy to implement. I have now uploaded my edits to this PR. Please gitclone/install from my fork branch to test it out. It works fine in my testing. All unit tests still passed without any issue.

For users using the Web UI has the option to input the PORT number as a parameter. The default is 2025.

For users, who run ConfigCreator.sh from a terminal, have the option to input the PORT number after the ipAddress in the format 192.168.1.3:2025. If not specified, the default is 2025.

Perfect, I did not think it would be too hard to tack this on. I am loving that this recent UI work is giving us back a lot of the flexibility and customisation that was lost from from very initial work as I transitioned to a plug-in. Very good thinking on the terminal side of things!

I do think most users either have all zones are with temperature sensors or all zones are without temperature sensors.

Some users will only have some zones with temperature sensors, so we can't forget about these people with mixed cases; I was one of these people myself when we built the house as I did not want to drop all the cash straight away to do the whole house and only did the more important zones.

The only minor drawback is that if the user does change the % zone open on those zones with temperature sensor, Cmd4 will send the Set request and will have to depend on AdvAir.sh to reject it. That was the motivation for me to give it a "code" (Zone-T instead of Zone), so that Cmd4 can identify it and not send any Set request at all for % zone open for zones with temperature sensors.

With the catching up I am doing, is this still an issue for us now with the linkedType method? It does potentially feel like something we should handle in AdvAir vs Cmd, but would be a lot more work I am assuming.

This is certainly a better way to clearly identify which zone has temperature sensors and which zone does not. Shall we implement that? It is not hard to do at all. The only modification is in ConfigCreator.sh.

I think I had shown you a screenshot of this a while back when I was fiddling with the base work of the stand-alone plug-in, what I should of mentioned was yes we can do that with this plug-in if we used linkedTypes; but it looks like you have worked that out already :) I do like this look and was thinking about it for the stand-alone plug-in, my only concern at all for our users of this plug-in is that if we implement it that we do not break existing setups and people who may only update but not re-run the ConfigCreator for the new configs... I think it should potentially be another config option box.

uswong commented 1 year ago

I do like this look and was thinking about it for the stand-alone plug-in, my only concern at all for our users of this plug-in is that if we implement it that we do not break existing setups and people who may only update but not re-run the ConfigCreator for the new configs.

All the changes we have done so far do not affect the existing users. Everything will run as it was for them with the upgrade. They have to re-run the ConfigCreator before they can use the new features.

With my successful test of using linkedTypes to include temperature sensor in one single homekit tile, I was thinking we should then expand it to one more item, ie the myZone switch. myZone is currently a separate switch in Homekit.

I looked through the Homebridge API and found Fanv2 may be suitable to also include myZone switch. Type Fanv2 has more characteristics than Fan or Lightbulb and I reckon Fanv2 SwingMode characteristic is probably the best to use as proxy for the myZone switch.

So I actually coded in additional characteristics (ie Active for On/Off, RotationSpeed for zone % open and SwingMode as myZone switch) in AdvAir.sh today and put together a sample config to test. It actually works! The round button at the bottom is SwingMode used as myZone on/off.

Do u still remember a couple of weeks ago, one user was actually suggesting to use Fan for Zone Control? we did not like it as we reason that it will get confused with the Aircon Fan. It looks like Fanv2 after all is the best choose to have all the controls pertaining to a zone in one Homekit tile.

So I am now actually suggesting that instead of using Lightbulb as a proxy for all Zone Controls, the option should change it to use Fanv2 as a proxy for all Zone Controls. What do you think?

Users can choose not to use this new feature, everything will run as it was even with the upgrade.

WhatsApp Image 2023-07-18 at 19 08 32

uswong commented 1 year ago

The only minor drawback is that if the user does change the % zone open on those zones with temperature sensor, Cmd4 will send the Set request and will have to depend on AdvAir.sh to reject it. That was the motivation for me to give it a "code" (Zone-T instead of Zone), so that Cmd4 can identify it and not send any Set request at all for % zone open for zones with temperature sensors.

With the catching up I am doing, is this still an issue for us now with the linkedType method? It does potentially feel like something we should handle in AdvAir vs Cmd, but would be a lot more work I am assuming.

It is not a linkedTypes issue. The minor drawback is related to the name Zone vs Zone-T.

In any case, I have already coded in AdvAir.sh to reject any Zone % open Set request from Cmd4. If we can reject those Set requests within Cmd4, it will improve the performance of the plugin. My enhanced Cmd4 module will do so if it can determine the Set request is sent from the zone with temperature sensor. That is why I initially would like to use Zone-T to identify the zone with temperature sensors. As I said, it is very minor! Don't worry about it.

mitch7391 commented 1 year ago

Just installed your latest version on my machine now. I did encounter an issue right off the bat, by doing nothing more than opening the settings and running the ConfigCreator without changing anything.

image

I thought strange at first and then realised that with the new addition of the PORT field, it would not have been saved into my config yet, I clicked save for it to save the default IP address and relaunched settings to try the ConfigCreator again and it worked. I can guarantee this will catch out a couple of people. Do we just cop that on the chin or do we change up the message to specify that it is missing the expected PORT number? Or... Is it possible that if the field is missing completely from the config, that it will still default it to 2025? This of course is a lot more work...

With my successful test of using linkedTypes to include temperature sensor in one single homekit tile

I did not see any of the linkedType work in there though, is this something you have not updated on your fork yet? I think adding the temperature sensor to the zone switch/lightbulb as a config option will be great. Again, it is coming down to the flexibility and customisation that users can choose to use or not without affecting all users.

Myself personally I tried the lights and went back to the switches as one, I do not control the percentage, and two, I did not like that HomeKit now thinks they are lights; HomeKit tells me how many lights are on in my house and if I use a blanket command of "turn off all lights", Siri will close these zones. But, if others like this, then it is great they have the option! :)

The Fanv2 stuff I think I may have to fiddle and try out myself as well to better get my head around it; which is harder without myZone myself. My first concern is that it is not at first obvious to the user what that button is and relies on them to have read the documentation to know that is what it does. Counter argument to myself here is, well Mitch we already do this with the Thermostat Auto mode...

So I am now actually suggesting that instead of using Lightbulb as a proxy for all Zone Controls, the option should change it to use Fanv2 as a proxy for all Zone Controls. What do you think?

I think let's add it as another option for people to have; switch, lightbulb, fan. Thoughts?

In any case, I have already coded in AdvAir.sh to reject any Zone % open Set request from Cmd4. If we can reject those Set requests within Cmd4, it will improve the performance of the plugin. My enhanced Cmd4 module will do so if it can determine the Set request is sent from the zone with temperature sensor. That is why I initially would like to use Zone-T to identify the zone with temperature sensors. As I said, it is very minor! Don't worry about it.

This sort of stuff will be a thing of the past once I get my butt in gear and get our stand-alone plug-in working!

uswong commented 1 year ago

Hey Mitch, good catch on the case of not having the PORT parameter saved first!

Or... Is it possible that if the field is missing completely from the config, that it will still default it to 2025?

Definitely, that is what I am going to do.

I did not see any of the linkedType work in there though, is this something you have not updated on your fork yet? I think adding the temperature sensor to the zone switch/lightbulb as a config option will be great.

That is correct. The linkedTypes testing I did was done manually but will include it in ConfigCreator as an option for users.

Thanks for your thoughts. I agree it is good to have all the options available to users as every user has their own preference. I have completed all the coding required on AdvAir.sh (the latest yet to be uploaded to my fork) but just the various options of how to put the config.json together which is a matter of manipulation within ConfigCreator.

What I can think of now are the following 4 options:

  1. using Switch/Lightbulb as proxy for Zone Controls with standalone temperature sensor and separate myZone switch (legacy).
  2. using Switch/Lightbulb as proxy for Zone Controls integrating temperature sensor as linkedTypes but still with separate myZone switch.
  3. using Lightbulb as proxy for all Zone Controls integrating temperature sensor as linkedTypes but still with separate myZone switch.
  4. using Fan/Fanv2 as proxy for Zone Controls integrating both temperature sensor and myZone switch as linkedTypes.

Option 1 is what we are using now. Option 2 and option 3 only affect users with temperature sensors.

For users without temperature sensors, option 1, 2 and 3 make no difference.

mitch7391 commented 1 year ago

Hey Mitch, good catch on the case of not having the PORT parameter saved first!

I am good at breaking things or finding ways to break them lol

Thanks for your thoughts. I agree it is good to have all the options available to users as every user has their own preference. I have completed all the coding required on AdvAir.sh (the latest yet to be uploaded to my fork) but just the various options of how to put the config.json together which is a matter of manipulation within ConfigCreator.

I have been thinking about it for a while, but I think we are now getting to a stage where I am comfortable to remove the config samples in the code base and even thinking we do not need the wiki section explaining the config in such detail; I will not delete this but maybe store it elsewhere so it does not overload users with info they do not need to worry about.

What I can think of now are the following 4 options

I am liking what we have here Ung Sing!

uswong commented 1 year ago

Hey Mitch,

I have completed the modification of ConfigCreator.sh for the various Zone Control setup options and have uploaded all that I have done. I have fully tested it also by gitClone/install from my latest fork branch into my Rpi and iMac and all seem to work fine. All 106 unit tests passed too. I have added a few more unit tests to test the Active and SwingMode characteristics.

I have also updated some parts of the documentation.

Please test it out for yourself. Please break it if you can before any user does... haha..

And please go ahead to merge it when you are happy with it.

mitch7391 commented 1 year ago

Hey Ung Sing, started with some quick testing for now and it is looking great! First I was not able to break anything on the UI haha the new coding for leaving the PORT blank works perfectly and testing each version of the config options worked great as expected! What I think I will want to do next is actually carry each of these configs through to HomeKit (not just view them on Homebridge and then blow them away) and see what they look and feel like; this will take a bit more time to test and will try to get to it today or tomorrow.

I might have some minor changes for the wording of our options, but that can come after the merge worst case. I already need to do some dependency updates anyway once it is merged so GitHub stops hounding me via email about them lol

uswong commented 1 year ago

What I think I will want to do next is actually carry each of these configs through to HomeKit

Hey Mitch, to allow you to have more extensive testing other than using your own system, I have uploaded two scripts called StartServer.sh and StopServer.sh which will allow you to start and stop the AirConServer. Once the AirConServer is started you will have a virtual AA system for you to test. What sort of system depending on the myAirData you load.

Do a git clone again from my fork branch:

cd rm -rf homebridge-cmd4-AdvantageAir git clone https://github.com/uswong/homebridge-cmd4-AdvantageAir.git cd homebridge-cmd4-AdvantageAir npm install --include-dev cd test chmod +x StartServer.sh StopServer.sh ./StartServer.sh myAirDataWith3noSensors.txt

The last statement will start the AirConServer (ip address: 127.0.0.1) with myAirDataWith3noSensors.txt which has one aircon system (ac1) with 10 zones. Zones 5, 6 and 10 are without any temperature sensors. The rest of the zones has temperature sensors and myZone is defined with myZone=7.

Once the AirConServer is started then you can just used it as an AA system with an ip address of 127.0.0.1.

You have to leave the AirConServer running throughout your testing.

When you are done with your testing, do the following to stop the server.

./StopServer.sh

Make sure to stop the server first before starting it again with other myAirData. You also need to stop it before you do any unit tests.

I might have some minor changes for the wording of our options, but that can come after the merge worst case.

I think it is probably easier for me to do the edits. There are two places we need to change those wordings. One is in index.html for the Web UI and the other is within ConfigCreator for users who needs to run it from a terminal. Please feel absolute free to let me know how you would like it to be worded.

uswong commented 1 year ago

Once the AirConServer is started you will have a virtual AA system for you to test. What sort of system depending on the myAirData you load.

Hey Mitch,

if you want a complete AA virtual system to test, you can use myPlaceFull.txt

./StartServer.sh myPlaceFull.txt

This virtual AA system has two aircon systems (ac1 and ac2), ac1 has 10 zones all without temperature sensors and ac2 has 10 zones too but with temperature sensors and myZone=7. It has a Gate and a Garage Door and 108 lights. Among 108 lights, there are 49 lights with dimmer. So this is really a very complete AA virtual system to test out every functionality of our plugin.

mitch7391 commented 1 year ago

Oooo I should actually give this a go as I have not played with the virtual AirconServer yet; or have not needed to. I have had a busier weekend and Monday than I expected, but should be able to give this a go tomorrow :)

I think it is probably easier for me to do the edits. There are two places we need to change those wordings. One is in index.html for the Web UI and the other is within ConfigCreator for users who needs to run it from a terminal. Please feel absolute free to let me know how you would like it to be worded.

Yep definitely get it will be easier for you to do it, I was thinking I wanted to have a fiddle and try/learn to do something new in GitHub which if done correctly could let me add changes to your PR; maybe I might leave it for another time though, see how I go haha

mitch7391 commented 1 year ago

Question for you @uswong, how do we visualise the virtual server? Originally for some reason in my mind I thought it was going to slap them into HomeKit in the Default Room, now I am thinking it will actually just display the jason body for me at the IP address specified (which in hindsight makes more sense now that I think about it)... It does look like the command is running correctly, but I cannot get any data at http://127.0.0.1:2025/getSystemData.

pi@homebridge:/tmp/homebridge-cmd4-AdvantageAir/test $ ./StartServer.sh myAirDataWith3noSensors.txt

up to date, audited 351 packages in 3s

51 packages are looking for funding
  run `npm fund` for details

5 moderate severity vulnerabilities

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
myAirData=myAirDataWith3noSensors.txt
pi@homebridge:/tmp/homebridge-cmd4-AdvantageAir/test $
uswong commented 1 year ago

Once the AirConServer is started (and it certainly looks like you have started it), the AirConServer will behave like an AA tablet with an ipAddress of 127.0.0.1. To visualize the data you have to use the AA API to send a Get request and you can also send a Set request too.

but I cannot get any data at http://127.0.0.1:2025/getSystemData.

Have you tried this on a terminal?

curl -s -g http://127.0.0.1:2025/getSystemData

image

mitch7391 commented 1 year ago

curl -s -g http://127.0.0.1:2025/getSystemData

Yep, there we go! Haha that now makes way more sense that the virtual server is isolated to the RPi dev system… This is very handy! Although some of the testing I want to do is seeing it in HomeKit and using it via the UI.

I’m already thinking for a future update for us would be to see if we can expose the server to HomeKit while we are running it… I won’t put much thought into it right now, but if we already have a server exposing the API, I do not see why not!

uswong commented 1 year ago

I’m already thinking for a future update for us would be to see if we can expose the server to HomeKit while we are running it…

this is exactly what the AirConServer can do.

Define a second AA device with name: AirConServer, ipAddress: 127.0.0.1, and Port used: 2025, then reboot homebridge. You will have your own AA system and this virtual one in your homekit.

mitch7391 commented 1 year ago

Of course... I don't know what I would do without you Ung Sing! :D

uswong commented 1 year ago

Define a second AA device with name: AirConServer, ipAddress: 127.0.0.1, and Port used: 2025, then reboot homebridge. You will have your own AA system and this virtual one in your homekit.

haha.... I meant to write:

Define a second AA device with name: AirConServer, ipAddress: 127.0.0.1, and Port used: 2025, run ConfigCreator, then reboot homebridge. You will have your own AA system and this virtual one in your homekit.

Try to use various Zone Control options and you will see what the systems with temperature sensors and myZone defined look like on homekit.

uswong commented 1 year ago

Hey Mitch, how did it go?

One question for you. How do you get a file in GitHub to be executable file like what you have done to AdvAir.sh, ConfigCreator.sh?

I am thinking of putting both StartServer.sh and StopServer.sh in the GitHub Repo permanently and would like to make it executable, so that when we git clone it, it will be executable by default.

mitch7391 commented 1 year ago

Hey Mitch, how did it go?

Only just fiddling around with it now and of course finding ways to break things :D I did not think I was going to like the Fan/Fanv2 option for myself (I get stuck in my ways of how I have always used this); but I can definitely see the appeal for new users... I never liked combined accessories when they first came out but they are growing on me (I prefer to click on to turn on the zone rather than click twice).

I wanted to see what the ConfigCreator would do with a second aircon as I have never tried this before; I was hoping it would apply the config changes to the Device tab I had open (one aircon and not both) as for this testing I am trying to leave my actual aircon alone. We of course do not ever need such a feature outside our testing and users will always want the config choices applied to both of their aircons. An easy for fix for me was copying and pasting parts of my old config back over it so my actual aircon does not change config. The next test for breaking something was to see how AdvAir.sh handled having two aircons as two different config setups and that also seems to be fine!

One question for you. How do you get a file in GitHub to be executable file like what you have done to AdvAir.sh, ConfigCreator.sh?

This has to be done on your test environment after you have used git clone.

  1. Navigate to your now local repo.
  2. Make your changes there (chmod +x StartServer.sh StopServer.sh).
  3. Run git status to see your changes.
  4. Run any tests if you want to be extra sure.
  5. Run git commit to commit your changes.
  6. Run git push to push it to your master. You more than likely have to be logged in for this from memory to prove it is you pushing changes to you repo (npm login and it will be your GitHub creds).

Let me know if you have any issues there, but I can also do it for you. As I said, I was going to try that GitHub thing that lets me submit changes to your PR before we merge it.

I am thinking of putting both StartServer.sh and StopServer.sh in the GitHub Repo permanently and would like to make it executable, so that when we git clone it, it will be executable by default.

Sounds perfect to me!

mitch7391 commented 1 year ago

The next test for breaking something was to see how AdvAir.sh handled having two aircons as two different config setups and that also seems to be fine!

I spoke to soon on this part... It will let me have my Aircon as legacy and the AirconServer as Fan/Fanv2, but a combo of any of the other kills cmd4 lol the point being that I am trying to keep my Aircon on legacy while I try the other options for the AirconServer.

uswong commented 1 year ago

It will let me have my Aircon as legacy and the AirconServer as Fan/Fanv2, but a combo of any of the other kills cmd4

It shouldn't in theory as both are independently of each other. Check your config to see if both are using the same key? one should be $AAIP and the second one should be $AAIP2.

To do a mix, I would suggest you generate the config for your own system first, copy the config and save it somewhere. Then generate both systems at the same time with your own system as the first system. After that, copy the accessories part of the first config to replace the accessories your own system of the second config. This is probably exactly what you have already done.

Would you mind sending me the mixed config that failed?

mitch7391 commented 1 year ago

To do a mix, I would suggest you generate the config for your own system first, copy the config and save it somewhere. Then generate both systems at the same time with your own system as the first system. After that, copy the accessories part of the first config to replace the accessories your own system of the second config.

Exactly what I am doing, we never need it to be able to do what I was saying out of the rare testing like this.

It shouldn't in theory as both are independently of each other. Check your config to see if both are using the same key? one should be $AAIP and the second one should be $AAIP2.

It was really chucking a fit but blowing it all away and starting again worked; has happened twice now, cannnot find any fat finger mistakes that caused it, but the fact that it is fine again after a clean slate shows it is not a functional error. Just ignore this for now haha...

uswong commented 1 year ago

I was hoping it would apply the config changes to the Device tab I had open (one aircon and not both)

haha... that would be ideal, Unfortunately config.schema.json, where the devices are defined, and index.html, where the options are selected, don't talk to each other. config.schema.json simply passes the data to index.htm.

I agree that users won't be having one setup for one aircon and a different set up for the other aircon. Not many users have two independent aircon systems also.

uswong commented 1 year ago

Let me know if you have any issues there, but I can also do it for you. As I said, I was going to try that GitHub thing that lets me submit changes to your PR before we merge it.

I got error in authentication!

image

It appears it did not like my password, so I generated a token but still not good.

image

Can you please do that for me?

mitch7391 commented 1 year ago

Strange, leave it with me :) gives me a good excuse now to play around with what we can do with GitHub!

mitch7391 commented 1 year ago

Ok, had a good play with our new config options! First off I’ll say I’m really impressed with this virtual server and what we can do with it to ensure we have completely tested our work for our users, especially when we do not have the devices they have!

An interesting one I just noticed that HomeKit does, is once we have combined the temperature sensor linkedType, it now says “Powered Off/On” instead of “Off/On” for switches. First screenshot shows what I mean, second is my untouched aircon.

IMG_6369

IMG_6370

I really like the temperature sensor linkedTypes added, I’m glad we didn’t wait for the standalone plug-in for this.

Another thing is the feedback message is now quite long and cut off. While we can scroll it sideways, users might not do that and will not see the more important end of the full message. What are your thoughts here?

image

INFO: An enhanced version of "Cmd4PriorityPollingQueue.js" was located and copied to Cmd4 plugin.DONE! Restart Homebridge/HOOBS for the created config to take effect OR run CheckConfig prior (recommended)
uswong commented 1 year ago

I really like the temperature sensor linkedTypes added, I’m glad we didn’t wait for the standalone plug-in for this.

Excellent! :)

feedback message is now quite long and cut off.

oh... I could never get my Web UI to copy the enhanced version of CmdrPriorityPollingQueue.js, so the message was meant for those who runs the ConfigCreator from a terminal. If some users can have that run within Web UI, I would simply take that message off leaving only the more important message for the feedback message.

I will upload a new version of ConfigCreator.sh in the next half hour or so with the above edits.

uswong commented 1 year ago

Hey Mitch,

New version of ConfigCreator.sh uploaded.

git clone again, and there will be no long messages in the feedback.

mitch7391 commented 1 year ago

I will take a look at it tomorrow Ung Sing, I will have to put this down for today... I also attempted to push that change for you into your PR and it rejected me. I think there is a setting that you need to make sure if ticked in your PR when you edit it:

"As a contributor, you need to un-check the Allow edits by maintainers box in PR page to disable maintainer's pushing permission, and it is checked by default."

Or maybe it had a fit due to you get that change done so quickly and changing the code base. I will try again tomorrow!

uswong commented 1 year ago

I also attempted to push that change for you into your PR and it rejected me. I think there is a setting that you need to make sure if ticked in your PR when you edit it:

"As a contributor, you need to un-check the Allow edits by maintainers box in PR page to disable maintainer's pushing permission, and it is checked by default."

It is checked! It should be checked if I would like you to push to it, right?

image

mitch7391 commented 1 year ago

Perfect, must have been that you beat me to the commit and changed the branch I was using haha

mitch7391 commented 1 year ago

Couldn't help myself, notes I was following were just a bit vague for a novice... It turns out I was getting a command wrong, not that the error message told me so lol looks like I have done it! Did you want to check and see if you are happy with the files being made executable?

uswong commented 1 year ago

yay! you have done it! Thanks.

image

uswong commented 1 year ago

I never liked combined accessories when they first came out but they are growing on me (I prefer to click on to turn on the zone rather than click twice).

Do you mean you have to click twice to turn on the zone for Fan/Fanv2 setup?

On my testing, I just need to click on the blue fan icon of the Homekit tile to turn on the zone. Only if I have to set the zone as myZone, then I click on the tile first then switch on the <> switch to set that zone as myZone.

mitch7391 commented 1 year ago

Hey Ung Sing, yeah I had forgotten that clicking on the icon inside the box vs clicking the box still turns the device on/off haha but you are right about the myZone; which is not a worry as the user would be using the main accessory more often than the linked accessory.

I am not sure if I said above with everything else, I really like the new Fan/Fanv2 setup; I am still not sure if I will migrate to it but I like it. Again for me, it comes down to what Siri thinks the accessory is and how she confuses it for others of the same type in a room. I will try it out and see if it works for my setup :)

mitch7391 commented 1 year ago

I will add in a few changes today as well, I have a few dependency updates that GitHub is hounding me over.

I think it is a little scary that Git can allow me to make changes to your fork, but I think it is only due to me being the originator, you having that check box ticked and because you have submitted a PR; I do not think I could otherwise. I wish GitHub web UI would let the originator make changes to a submitted PR though and not have to do it through your dev system. I don't think we will continue to do it this way, but it was a fun little test for this PR.

mitch7391 commented 1 year ago

@uswong I have moved the checked box on our Zone Control Options to the 'legacy' option. The reasoning behind this being that the user will already be on 'legacy' and if they accidentally hit the ConfigCreator button, it will effectively just recreate the config they already have; think of the new features as more of a 'opt in' situation. Let me know if you disagree and we can always revert.

uswong commented 1 year ago

think of the new features as more of a 'opt in' situation.

Agreed!

uswong commented 1 year ago

I will change the default to option 1 in ConfigCreator.sh for those who runs ConfigCreator from a terminal.

mitch7391 commented 1 year ago

I must admit I have not run the terminal ConfigCreator in some time, I did this one so quickly as I did not think there would be default/checked options there; sorry for not double checking this one!

uswong commented 1 year ago

I have not run the terminal ConfigCreator in some time, I did this one so quickly as I did not think there would be default/checked options there; sorry for not double checking this one!

No worries Mitch. Do you want to change the wordings of the various zone control options? The wordings are exactly the same in both index.htm and ConfigCreator.sh. Please feel free to edit it using your github dev system.

mitch7391 commented 1 year ago

I had run the terminal version after my last comment and like the additions to it since it was first created.

Do you want to change the wordings of the various zone control options?

I will have time to do this in the second half of tomorrow. After that we should be good to merge and look at publishing :)

uswong commented 1 year ago

Hey Mitch,

Please have a look also on item 10 and item 12 of the README to see if some of the wordings need to be altered to make it clearer. English is my 3rd language, so it is always good to be checked by a native English-speaking person. Thanks.

mitch7391 commented 1 year ago

Hey @uswong I have made the very minor changes to the wording of the Zone Control Options, the main change is adding an asterisk that better explains what "Lightbulb/Switch" entails and changing 'as proxy' to 'accessory'. Oh, I have also changed 'Fan/Fanv2' as just 'Fan' as our users do not know cmd4 like we do, to them they just want to know this is a fan icon on HomeKit.

image

image

I have changed the wording for this one too:

image

I have tested these work for me, please test that they work for you too or let me know if you want to revise how I have worded the changes. Once you are happy, we will go for the merge, I can then fiddle with the README and any changes required via GitHub later if they are required.