TJForc / risco-lan-bridge

risco-lan-bridge is a "bridge" between a node.js application and a Risco Panel
Other
21 stars 12 forks source link

Panel type and options discovery #9

Open vanackej opened 2 years ago

vanackej commented 2 years ago

Implement Panel type and options discovery.

I also introduced some methods on TcpSocket in order to avoid code duplication for command results management

vanackej commented 2 years ago

Hi @TJForc Thanks for the review.

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, ...)

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?

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.

This would avoid cascading function calls.

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

The comments of the functions also contain an error, you do not return Promise but either a string or a number.

Yes I can fix the comments

Otherwise in principle, the idea suits me.

vanackej commented 2 years ago

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 commented 2 years ago

@TJForc I made an update, trying to take into account most of your feedbacks. Let me know if it looks better for you or what I could do next. In the meantime, I will publish my own version of this library in order to allow further home assistant integration and get feedbacks from the community. My intent is not to fork the library, but I want to move forward either way.

vanackej commented 2 years ago

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. I'd also like to discuss how/if you want to maintain the library and if you want us to share the responsibility for this. Would you be available for a little chat any day soon ? If not that's ok, I will just maintain a fork on my own, without the proxy mode as I can't test it

pergolafabio commented 2 years ago

Keep me posted , really want to use proxy method... For now I stopped using the addon because of the proxy issues :-(

robertboccia commented 2 years ago

My advise is to team up with @OnFreund as he owns the official home assistant intergration

Robert Boccia


From: pergolafabio @.> Sent: Tuesday, December 28, 2021 1:17:40 AM To: TJForc/risco-lan-bridge @.> Cc: Subscribed @.***> Subject: Re: [TJForc/risco-lan-bridge] Panel type and options discovery (PR #9)

Keep me posted , really want to use proxy method... For now I stopped using the addon because of the proxy issues :-(

— Reply to this email directly, view it on GitHubhttps://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FTJForc%2Frisco-lan-bridge%2Fpull%2F9%23issuecomment-1001795453&data=04%7C01%7C%7C5a44c0653867489b654108d9c98f145d%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637762438614950194%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=ZBg0CTRGCS6laPs1EtGUIrRlPmN9w0%2FDb803IZI8tBw%3D&reserved=0, or unsubscribehttps://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAR4IWBE3LWI4FOXO6FXF4UDUTDXZJANCNFSM5HNSNOQA&data=04%7C01%7C%7C5a44c0653867489b654108d9c98f145d%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637762438614950194%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=DvIZLtgLYPxjKRCbKrF6t9JftOkfKEAVqx72h0uq7cU%3D&reserved=0. You are receiving this because you are subscribed to this thread.Message ID: @.***>

vanackej commented 2 years ago

@pergolafabio I implemented proxy mode yesterday. Tryied it locally, works with my setup However, I would strongly advise against using it. If Risco cloud or your internet connection is down, the library won't be able to start and you won't be able to control your panel using home assistant. Also, my risco cloud cloud access will expire on 01/01/2022, so I won't be able to test anything past this date. Maintenance will be in best effort mode.

@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.

pergolafabio commented 2 years ago

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?

vanackej commented 2 years ago

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 https://github.com/TJForc/risco-lan-bridge/pull/9#issuecomment-1002530147, 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: @.***>

pergolafabio commented 2 years ago

Ok, will do next week, thnx for all updates!!

pergolafabio commented 2 years ago

@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