Closed ztalbot2000 closed 2 years ago
mitch7391
commented
2 years ago Thanks for this @ztalbot2000, I will take some time to look over this when I can. I fear my biggest battle is going to be to get GitHub to like it as it has a number of conflicts due to the fact I went ahead with v3.3.0 before taking this on.
@uswong can you please review alongside me as well? Always best to get more eyes looking over this considering my level of experience vs the two of you.
ztalbot2000
commented
2 years ago Hi Mitch,
Surprisingly there are actually no conflicts. I had propped your latest changes to my branch, but Git is confused. I will do the merge if that is okay with you.
John
On Fri, Apr 15, 2022 at 12:20 PM Mitch Williams @.***> wrote:
Assigned #55 https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/55 to @ztalbot2000 https://github.com/ztalbot2000.
— Reply to this email directly, view it on GitHub https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/55#event-6441148439, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSBCX2H4O6N6TCUEEDZHIDVFGJMHANCNFSM5TONPF3A . You are receiving this because you were assigned.Message ID: <mitch7391/homebridge-cmd4-AdvantageAir/pull/55/issue_event/6441148439@ github.com>
mitch7391
commented
2 years ago Hmm that is strange then John! You can probably see this yourself, but it is showing me the following issues:
It states that these conflicts cannot be resolved in the web editor and it will require the command line to resolve. I am happy to leave this with you if you think you can get around it; I know you have done this a lot more than me haha...
ztalbot2000
commented
2 years ago Hi Mitch,
Yeah I know. Git compare is sometimes useless. For example some of the files you listed are new or removed, yet Git complains.
Ttyl, John
On Fri, Apr 15, 2022 at 12:39 PM Mitch Williams @.***> wrote:
Hmm that is strange then John! You can probably see this yourself, but it is showing me the following issues:
[image: image] https://user-images.githubusercontent.com/40288237/163596964-310856ab-4e20-408f-93ce-d66a93dda03a.png
It states that these conflicts cannot be resolved in the web editor and it will require the command line to resolve. I am happy to leave this with you if you think you can get around it; I know you have done this a lot more than me haha...
— Reply to this email directly, view it on GitHub https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/55#issuecomment-1100218230, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSBCX7GMQPUUDTNIMIHHWDVFGLVBANCNFSM5TONPF3A . You are receiving this because you were mentioned.Message ID: @.***>
ztalbot2000
commented
2 years ago Hey Mitch,
Most of those files you never touched to. AdvAir.sh is the only one you really care about.
I will do the merge for you into your beta branch.
Ttyl, John
On Fri, Apr 15, 2022 at 12:41 PM John Talbot @.***> wrote:
Hi Mitch,
Yeah I know. Git compare is sometimes useless. For example some of the files you listed are new or removed, yet Git complains.
Ttyl, John
On Fri, Apr 15, 2022 at 12:39 PM Mitch Williams @.***> wrote:
Hmm that is strange then John! You can probably see this yourself, but it is showing me the following issues:
[image: image] https://user-images.githubusercontent.com/40288237/163596964-310856ab-4e20-408f-93ce-d66a93dda03a.png
It states that these conflicts cannot be resolved in the web editor and it will require the command line to resolve. I am happy to leave this with you if you think you can get around it; I know you have done this a lot more than me haha...
— Reply to this email directly, view it on GitHub https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/55#issuecomment-1100218230, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSBCX7GMQPUUDTNIMIHHWDVFGLVBANCNFSM5TONPF3A . You are receiving this because you were mentioned.Message ID: @.***>
mitch7391
commented
2 years ago Thanks John, let me know how you go or if you need me to do anything!
uswong
commented
2 years ago @uswong can you please review alongside me as well? Always best to get more eyes looking over this considering my level of experience vs the two of you.
I am super excited to see the creation of AirConServer. I have a go with it and it really behaves like a real small system. A big system is a lot more laggy.
I also have a good look over the AdvAir.sh and thanks JohnT for making the code a lot more elegant. However I have the following concerns for big MyPlace systems:
If a lock file is detected, the use of iteration to getSystemData may not be enough for a big system. A big system takes between 1s to 60s (histogram distribution shown below) to getSystemData, ie the lock file can be there on average ~6s and occasionally can be up to 60s and beyond. The maximum time allowed within the iteration is 4.8s. As such, for a big system, ~50% of getSystemData will fail.
The histogram below shows the time taken to getSystemData for a big MyPlace system. The overall average time taken is ~6s. It has ~6000 data points.
For a big system with a lot of automations programmed within the control unit, the curl output of getSystemData to a variable directly may be fragmented. This can be resolved by outputting the curl getSystemData result to a file first.
The "forced" getSystemData after every Set command will overwhelm the aircon system and will bring the control unit to a grounding halt if an user runs his automation to turn on/off multiple lights.
JohnW has an automation set up to turn off all his 77 lights when he leaves home. That will certainly bring his aircon control unit to a grounding halt and not all lights will be turned off. Moreover, the fact that the aircon control unit may take up to 4s to populate the json body after each Set command will further aggravate the performance.
May I propose to leave the above concerns to me to resolve? And I will do a PR when done and allow John to look over it and perhaps can help to make it more elegant in the coding.
Cheers, Ung Sing
ztalbot2000
commented
2 years ago Hi,
I don't remember coding a timer for the complete get to finish. Only a timer between tries.
Sets don't happen very often. Ideally an incoming Set would also update the local systemData so another fetch is not needed. A true daemon would always have the latest copy of data in memory. This is our next step. Remember Cmd4 waits for the Set to be started alone and foes not do anything else until it is done. This being true, Gets never need to update the systemData because Cmd4 holds the latest Set values in memory because of WoRm2. AdvAir.sh still reads and writes the systemData to a file. The Get will read from this file first according to your timing rule. Your code in Gets read the full systemData again in many places, just not in all. As was the case, it was better to do the full read first. We had figured out much earlier what constitutes a successful read, not just a random file size. It would be very interesting to see how this AdvAir.sh behaves in a large system. I'd really like to see before any changes are made.
Ttyl, John
On Sat, Apr 16, 2022 at 3:56 AM uswong @.***> wrote:
@uswong https://github.com/uswong can you please review alongside me as well? Always best to get more eyes looking over this considering my level of experience vs the two of you.
I am super excited to see the creation of AirConServer. I have a go with it and it really behaves like a real small system. A big system is a lot more laggy.
I also have a good look over the AdvAir.sh and thanks JohnT for making the code a lot more elegant. However I have the following concerns for big MyPlace systems:
1.
If a lock file is detected, the use of iteration to getSystemData may not be enough for a big system. A big system takes between 1s to 60s (histogram distribution shown below) to getSystemData, ie the lock file can be there on average ~6s and occasionally can be up to 60s and beyond. The maximum time allowed within the iteration is 4.8s. As such, for a big system, ~50% of getSystemData will fail.
The histogram below shows the time taken to getSystemData for a big MyPlace system. The overall average time taken is ~6s. It has ~6000 data points.
[image: image] https://user-images.githubusercontent.com/96530237/163665522-18115246-f239-4077-995f-58d2624832ed.png
1.
For a big system with a lot of automations programmed within the control unit, the curl output of getSystemData to a variable directly may be fragmented. This can be resolved by outputting the curl getSystemData result to a file first. 2.
The "forced" getSystemData after every Set command will overwhelm the aircon system and will bring the control unit to a grounding halt if an user runs his automation to turn on/off multiple lights.
JohnW has an automation set up to turn off all his 77 lights when he leaves home. That will certainly bring his aircon control unit to a grounding halt and not all lights will be turned off. Moreover, the fact that the aircon control unit may take up to 4s to populate the json body after each Set command will further aggravate the performance.
May I propose to leave the above concerns to me to resolve? And I will do a PR when done and allow John to look over it and perhaps can help to make it more elegant in the coding.
Cheers, Ung Sing
— Reply to this email directly, view it on GitHub https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/55#issuecomment-1100599668, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSBCX46Y2LZGCUL6Y7MIN3VFJXDXANCNFSM5TONPF3A . You are receiving this because you modified the open/close state.Message ID: @.*** com>
mitch7391
commented
2 years ago Hey @ztalbot2000 sorry for the late reply on this one. I would like @uswong to move forward with the changes he has in mind. I can see that the proof is in the pudding with the data presented and as a user, I have noticed that the overall performance of all of our work combined is the best it has ever been. Definitely always happy for your suggestions of how we can better do things as all of us are always still learning (me the most out of the three of us!), but I am happy with the explanation Ung Sing has provided and am not sure if we are getting lost in translation at the moment :)
ztalbot2000
commented
2 years ago Hi all,
So please do not trash all my work without at least trying it in a large system and sending me the logs. For all you know, my answer may be the better and the correct one.
ttyl, John
On Tue, Apr 26, 2022 at 1:41 PM Mitch Williams @.***> wrote:
Hey @ztalbot2000 https://github.com/ztalbot2000 sorry for the late reply on this one. I would like @uswong https://github.com/uswong to move forward with the changes he has in mind. I can see that the proof is in the pudding with the data presented and as a user, I have noticed that the performance of all of our work combined is the best it has ever been. Definitely happy for your suggestions of how we can better do thing as all of us are always still learning (me the most out of the three of us!), but I am happy with the explanation Ung Sing has provided and am not sure if we are getting lost in translation at the moment :)
— Reply to this email directly, view it on GitHub https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/55#issuecomment-1110075504, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSBCX7AXSZWQTIJ43J3TALVHATDBANCNFSM5TONPF3A . You are receiving this because you were mentioned.Message ID: @.***>
mitch7391
commented
2 years ago Hey @ztalbot2000, I am sorry if it has come across that way. That being said, a lot of Ung Sing’s hard work was removed in your PR as well; but that had been extensively tested with data to back the results. I am sorry if we have not given your PR the same courtesy.
I cannot speak for Ung Sing, but I did not have a chance to test your PR as my personal life has been really hectic lately (as you both know); but I think Ung Sing had tested it for me. His son JohnW is the only one we can rely on to test a larger system, but he is a very busy man and any setbacks require remote support from Ung Sing to rectify. This being said, with Ung Sing’s work that was removed in your PR; I had never seen cmd4 respond so quickly to our script. But I can only speak for a smaller system.
I am a little bit between a rock and a hard place if the two of you do not agree with each other’s work and do ask that you both keep an open mind with each other’s work and try to keep any emotion out of it. I highly respect both of you, your opinions and really love how much the two of you have made this project what it is, for myself and the collective of users I now have. It is all thanks to you both.
ztalbot2000
commented
2 years ago Hi,
His features were good. His system design is not. His Histogram data is bogus because while they are data points of accessing from the AirCon, they are data points when some times the Sets were done during full Get, not because of a retrieval of updating the system data every two minutes, but some of his new features wanted a full Get anyway. I've stated all that before. So how about some new stuff. You keep so many copies of the getSystemData that it slowed down the system itself, skewing the histogram. Here is another big one. You are writing data on the Pi's flash card, which is extremely slow! What class type was the flash card on all his system tests? The flash card slows down his histogram data? How about all the Unit tests of mine that were broke? Many of which showed that the new code added was faulty. I'm just looking for the same consideration. Especially since it is not even been tried to see if what I wrote is better. Just think, I've had to understand this thing in my mind for two years, not even having one.
What is the harm in trying! If nobody is willing to do that then I cannot help you anymore.
Ttyl, John
On Sat, Apr 30, 2022 at 8:50 PM Mitch Williams @.***> wrote:
Hey @ztalbot2000 https://github.com/ztalbot2000, I am sorry if it has come across that way. That being said, a lot of Ung Sing’s hard work was removed in your PR as well; but that had been extensively tested with data to back the results. I am sorry if we have not given your PR the same courtesy.
I cannot speak for Ung Sing, but I did not have a chance to test your PR as my personal life has been really hectic lately (as you both know); but I think Ung Sing had tested it for me. His son JohnW is the only one we can rely on to test a larger system, but he is a very busy man and any setbacks require remote support from Ung Sing to rectify. This being said, with Ung Sing’s work that was removed in your PR; I had never seen cmd4 respond so quickly to our script. But I can only speak for a smaller system.
I am a little bit between a rock and a hard place if the two of you do not agree with each other’s work and do ask that you both keep an open mind with each other’s work and try to keep any emotion out of it. I highly respect both of you, your opinions and really love how much the two of you have made this project what it is, for myself and the collective of users I now have. It is all thanks to you both.
— Reply to this email directly, view it on GitHub https://github.com/mitch7391/homebridge-cmd4-AdvantageAir/pull/55#issuecomment-1114086325, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSBCXYOYYDDWLAKZKDZG4DVHXILTANCNFSM5TONPF3A . You are receiving this because you were mentioned.Message ID: @.***>
name: Pull Request about:Resolve issues found through new AirConServer with all new unit tests title: "[Pull Request]" labels: pull-request assignees: mitch7391
Is your pull request related to a problem or a new feature? Please describe:
AdvAir.sh does many things that avoid proper testing of the complete script in a test environment. in doing so many kludges were created to work around proper system behaviour; some which I have determined can cause a Pi to crash.
Describe the solution you'd have implemented:
Do your changes pass local testing:
Additional context: