Panel type and options discovery #9
Conversation
vanackej
force-pushed
the
feature/panel_discovery
branch
from
4a36b7d
to
cb33e1f
Compare
There was a problem hiding this comment.
Commit " fix #7 Panel Type auto-discovery " and "Update examples"
This functionality will deserve its own development and will not be accepted as is.
Some changes deserve to be discussed before application such as:
- Removal of the function for assigning the SupportPirCam variable,
- Deletion of current exports
These changes incompatible with backward compatibility with plugins already using this library and require automatic mode as a standard, which is too early in terms of the development of this project given that communication with all types of control panel is not validated. .
This imposition of automatic mode and the deletion of the equipment classes prevents the addition of functions or other variables which may be necessary to adapt the communication in the case of different behavior of a central unit.
A depreciation period should also be applied to prepare for such large changes.
The manual and documentation will also need to be updated.
It is preferable that this feature has its own pull requests and that each modification is reviewed together.
There was a problem hiding this comment.
Commit "Introduce GetResult and GetIntResult functions in order to avoid code… "
Why create 2 different functions?
Why not test if the result is a number and return the result as a string or a number depending on this test?
This would avoid cascading function calls.
The comments of the functions also contain an error, you do not return Promise but either a string or a number.
Otherwise in principle, the idea suits me.
There was a problem hiding this comment.
Commit "Make watchdog interval configurable "
Ok in principle.
On the other hand, the definition of the variable seems to me misplaced and I don't like this syntax because it gave me problems when the variable is undefined.
For me, the definition should be placed higher in the class builder.
Then, the watchdog call will be made with this variable.
The idea is to define / assign the variables of the class in the same block of code.
|
Hi @TJForc Maybe I missed something about the SupportPirCam function, but given my comprehension, it's not used for now (as the computed result is always false). I also took care to keep the condtional tests and logs in order to keep the current behavior, even if not yet used. The function was necessary because instanciated before panel connection was initialized, but now that the PIR support is checked after panel connection, we have all the neccessary informations to check if PIR camera are supported (panel type, firmware versions, ...)
The command result is always a string, that have to be parsed as a number depending on the command. Sometimes a digits only number must be interpreted as a string (imagine a serial number with leading zeros for exemple). And it would also involve systematic number parsing attempt, even for basic string results. At last, I wanted the caller to be in charge of the expected command result type => The caller have knowledge of the expected result type and can perform results checks efficiently, not depending on how automatic conversation would be done.
I can avoid function cascading by duplicating some code, as you prefer. I personnaly prefer avoiding duplication than a simple cascade, but I don't really mind both options
Yes I can fix the comments
|
|
About the removal of current imports and backward compatibility : Just adding the RiscoPanel subclasses back would keep code backward compatibility for third party libraries. It would allow them for progressive migration to the discovery mecanism. At the end of the day, I really think they will be happy with panel type auto discovery. Having to do code like this does not help easy third party library integration : https://github.com/gawindx/homebridge-risco-local-platform/blob/53348523013561068e23b3506f81f966ad4318db/app.js#L108 |
vanackej
force-pushed
the
feature/panel_discovery
branch
from
7b0b8dd
to
576f026
Compare
|
@TJForc |
|
Hi I've worked a lot on this library to improve stability and fix bugs. I migrated the codebase to typescript. At this occasion I've catched many typos / typing issues / async issues. Most of them are fixed now in my codebase. But I struggle with the proxy mode, and would really appreciate having a discussion with you regarding this point. |
|
Keep me posted , really want to use proxy method... For now I stopped using the addon because of the proxy issues :-( |
|
@pergolafabio I implemented proxy mode yesterday. Tryied it locally, works with my setup @TJForc feel free to close/reject this PR. Again, I let you come back to me anytime if you want to talk about library maintenance and the fork I initiated. |
|
Hey, thnx in advance, but have you also had it running for more then about 30 hours? Seems when there is Somekind of interrupt somewhere, it crashes the container... I also created 2 issues on this GitHub page for it.. |
|
Just test and let me know. I tried to take care of disconnections on both
sides (panel and cloud).
Run with log in debug and attach logs if any crash happens.
If all ok please close the issues
Le mer. 29 déc. 2021 à 11:41, pergolafabio ***@***.***> a
écrit :
… Hey, thnx in advance, but have you also had it running for more then about
30 hours? Seems when there is Somekind of interrupt somewhere, it crashes
the container... I also created 2 issues on this GitHub page for it..
Do you think it will be resolved now with your new updates in typescript?
—
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAECPHODLZFALWVLKBUAULDUTLQW3ANCNFSM5HNSNOQA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Ok, will do next week, thnx for all updates!! |
|
@vanackej , are you also planning to put it on HACS, maybe on a custom repository for now? easier to track updates if you maka new release |
Implement Panel type and options discovery.
I also introduced some methods on TcpSocket in order to avoid code duplication for command results management