[Pull Request] Unit Test Server #55
Conversation
…the AirCon This makes testing behave more like a real system and no kludges for Unit Testing This update has only GetBrightness working, via npm run bats test/GetBrightness.bats
…lat files. This mimicks a real AirCon
…check older stuff
…y, shrinking duplicate tests
|
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 @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. |
mitch7391
added
enhancement
|
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
<#55> to
@ztalbot2000 <https://github.com/ztalbot2000>.
—
Reply to this email directly, view it on GitHub
<#55 (comment)>,
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>
|
|
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... |
|
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
<#55 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSBCX7GMQPUUDTNIMIHHWDVFGLVBANCNFSM5TONPF3A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
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
> <#55 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABSBCX7GMQPUUDTNIMIHHWDVFGLVBANCNFSM5TONPF3A>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
Thanks John, let me know how you go or if you need me to do anything! |
I am super excited to see the creation of 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:
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, |
|
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
<#55 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSBCX46Y2LZGCUL6Y7MIN3VFJXDXANCNFSM5TONPF3A>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***
com>
|
|
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 :) |
|
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
<#55 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSBCX7AXSZWQTIJ43J3TALVHATDBANCNFSM5TONPF3A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
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. |
|
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
<#55 (comment)>,
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: