Replies: 23 comments 78 replies
-
|
I pass all those problematic stuff to the Switcher devs. |
Beta Was this translation helpful? Give feedback.
-
|
So this is Switcher's reply: Regarding (1), They say that this Token is part of ensuring security as Runners are different from other "Normal" Switcher devices as they are more sensitive and we don't want attackers accessing our Runners remotely and messing with our house. They wanted to verify again that this Token is "Local Only" and not accessing the Switcher server by any means. About having an API call to get the Token - This is not possible because this will break the meaning of this Token to validate the user requiring the Token. Regarding HA stuff, They say that they can have an API call that gets the account owner's email and the Token is sent by email. Regarding (2), They say they probably can solve that and share the knowledge of the Token required by the device and firmware. I want to mention that Daniel the Owner of Switcher is trying to contact you guys and opened a WhatsApp group, So please get back to them. |
Beta Was this translation helpful? Give feedback.
-
|
I am on vacation, so expect slow responses until holidays are over.
I still don't understand what is "when Switcher decides to use Token for other devices.".
This is not acceptable, you can't ask users to add a token after they update (@TomerFi this would be the same for web_api) if they don't need it. This would mean their installation will break until they add a token and is not acceptable. Also there is no reason to do it if none of the devices require it.
I agree API call reduce the chance for error, but I still don't agree we don't need to validate the token. I asked earlier without a clear response, so I will try to reparse it.
Since Switcher already added support for an API, I would prefer it will be a part of the library, It can be part of the tools as suggested in #716 so that it doesn't run on runtime unless requested by the user (or HA integration). Library itself would still stay local only, but will provide a util to get the token. @YogevBokobza please consult with Switcher of a better way to identify if device needs a token or not. I would prefer an option that will do this based on communication with the device or something in the discovery from the device and not based on assuming something on the model type, same for validating the token. We need a better way to validate it, even if the API is automated. I don't want to deal with issues of users complaining the device is not controllable and we have nothing we can do to identify the problem. Thanks. |
Beta Was this translation helpful? Give feedback.
-
Of course Switcher do not update the devices by themself, But in a future versions of the devices Token will be needed.
As I said before it won't break anything existing. Even if you request login from the users it won't break anything because the packets that are sending will be the same. Currently token is needed if the user has one or more of this devices: Runner S11, Runner S12 and Switcher Lights (of any kind).
When you are using token it's a different packet sent. That's is why we have "is_using_token" logic to determine which packet should be sent.
This should not happen because "is_using_token" logic the specify which packet should be sent to the device. Its simple as that:
I am not sure about that.. I don't think so..
Exactly my attention with that PR.
They said that the best way right now is to ask for token if the user has one or more of this devices: Runner S11, Runner S12 and Switcher Lights (of any kind) Validation can be part of this library too but it will be shallow one.(like number of chars, symbols etc.) |
Beta Was this translation helpful? Give feedback.
-
Yesterday we had a report from a user that his Switcher Touch suddenly not controllable from HA, as per our discussion above we didn't find anything wrong, just that the device is not performing API commands anymore. Finally I suggested that maybe his device was updated and he called Switcher which confirmed that the device FW was automatically updated. Adding to that they stated that:
I am recapping here your suggested options:
Since this devices is Switcher Touch: Also Switcher response was that we are not waiting for anything from them and development is almost over, blocked only by lake of time, which I don't agree with. Our first step is to have a determined way to detect if a device needs a token or not and an option to validate the token (some way of a response from the device to indicate a token is invalid), otherwise we are going to deal with growing number of issues without any tools to identify the problem (same as happened yesterday) which is not acceptable. I understand you want to contribute and finish your development but Switcher needs to give more accurate information how to handle this or how they handle it in their app. |
Beta Was this translation helpful? Give feedback.
-
|
Regarding this:
I talked to the Switcher dev guy and tested it on my Swithcer Touch device.
This is a bug he is investigating. This is not related to any token logic. Regarding devices that are updating automatically by Switcher, I was not aware of that.. I am sure they have their reasons for that. The Switcher dev discussed this token stuff with his teammates and is offering this solution as a healthy way to do it. The solution is as below:
The Switcher dev tested this kind of solution and it was working for him and no delay in action occurred. |
Beta Was this translation helpful? Give feedback.
-
|
Sorry I was away for some time. I noticed there was some advancment made in the next comment. Please let me know if there's anything I can help with to advanced any of this. |
Beta Was this translation helpful? Give feedback.
-
|
Sorry, haven't had the time to go over this yet. I'll make some time this weekend. |
Beta Was this translation helpful? Give feedback.
-
|
Hey Guys, |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
|
I think vendor access to retrieve the token is acceptable and I have no problems merging this part, but don't get it wrong, we still have open issues how and when to use the token when we get it. |
Beta Was this translation helpful? Give feedback.
-
|
@TomerFi regarding #716 |
Beta Was this translation helpful? Give feedback.
-
|
@TomerFi @thecode I am saying let's merge #716 to the runner branch and we continue to discuss about token logic |
Beta Was this translation helpful? Give feedback.
-
|
@thecode I understand. In that case, I'll merge this, as @YogevBokobza to the runner-support branch. |
Beta Was this translation helpful? Give feedback.
-
|
Merged. |
Beta Was this translation helpful? Give feedback.
-
That would probably be the only clean option, since current implementation is async mixing the TCP & UDP is something we should avoid.
|
Beta Was this translation helpful? Give feedback.
-
|
I am having issues with rebasing runner-support.. I made this branch "runner-support-fixed" (Future PR for integrating to dev: #744) which is dev + the changes in runner-support. |
Beta Was this translation helpful? Give feedback.
-
|
So the original |
Beta Was this translation helpful? Give feedback.
-
|
@TomerFi Yes.. let's delete it then |
Beta Was this translation helpful? Give feedback.
-
|
@TomerFi I deleted that. :) |
Beta Was this translation helpful? Give feedback.
-
|
So now runner-support-fixed is the main one, and s11-support-fixed is next on the list to talk about |
Beta Was this translation helpful? Give feedback.
-
|
So let's get back to the Token part. So we agreed that the Token logic is essential for the Switcher team to ensure better security controlling our devices. What does the token logic affect? it added new packets that need to be sent to the devices that are using the token. So we have different packets sent between a device that uses a Token and one without it. The issue starts here where on the "set_position" part for example we are currently sending "DeviceType.RUNNER" as a type but this should be based on the actual device type (https://github.com/TomerFi/aioswitcher/blob/dev/src/aioswitcher/api/__init__.py#L716-L727) Currently, the only devices that need Token no matter which version are: Switcher Runner S11, Switcher Runner S12, and all Switcher Lights. Therefore, in the current state of the operating function, we don't know which device it is and we are unable to "choose" the correct packets to send. Also knowing the device_type like this will also solve this kind of issue #335 |
Beta Was this translation helpful? Give feedback.
-
Currently there are reports by users that new Switcher Touch doesn't work (and looking at the firmware version it looks like it needs a token), on the other hand older Switcher Touch (like mine) doesn't require a token, so I don't understand how you plan to do it by device type.
We have been in this discussion in the past without clear response from Switcher, according to multiple users who called Switcher support, if your device is in warranty period it will be automatically updated if it is out of warranty (like my devices) it will never be updated, if you want your device to stay on older firmware you need to ask to get your device out of warranty (seems strange). I would like to stop doing this discussion back and forth and keep discussing over and over on the same problems. I also don't want you to invest time without progressing. If Switcher wants support for their devices they will need to supply information for the following:
|
Beta Was this translation helpful? Give feedback.
-
|
I agree with @thecode. I don't think we should hold on making siginficent changes to the API types before getting some soft of confirmation on how to identify devices requireding a key. |
Beta Was this translation helpful? Give feedback.
-
|
@TomerFi @thecode Right now, only the devices mentioned require a Token. Let's say hypothetically we continue with the idea of Token based on device type, which of the PRs mentioned is the best way to approach this? (using existing api1 and 2 and adding a parameter, or Device class that encapsulates api1 and api2) |
Beta Was this translation helpful? Give feedback.
-
@TomerFi @thecode Update:
Regarding all of that, it is safe to say that we can apply the Token logic based on the user's owned devices. |
Beta Was this translation helpful? Give feedback.
-
|
I'm sorry - I'm a bit swamped - I will take a look at this later this week. |
Beta Was this translation helpful? Give feedback.
-
I asked them about it in the past they said it is possible but we didn't listen to the port the device advertise the firmware version (which I think we already added to the ports we listen, so I have some worries about the validity of the answers you get from them. I must say I am not happy with all this process, it is enough to read this discussion and see that there are contradictory answers already here. Uses who asked them also got confusing answers. I also don't understand how they will deal with it in their app, even if it will take a year they would still need to address different firmware capabilities. The App knows the device firmware and I assume they are not guessing it. I would still recommend you ask them (just send the questions and paste the answer here as it is):
However since you are eager to push this implementation if you want to add a flag which will be sent with the device discovery data that will mark if the device needs token you can go ahead with this implementation. |
Beta Was this translation helpful? Give feedback.
-
|
@thecode Anyway, I will send your questions and reply with their answers. So regarding the flag you mentioned, would it be nicer to have another tool function in this library that returns true/false if a device needs a token or not? (by looking at the device type) Also, we need to pass the Token itself into the api1 and api2 (well mainly api2 as for now) - For that we gonna need to know the device type upfront. |
Beta Was this translation helpful? Give feedback.
-
I Haven't changed my phone and didn't have a call from them for almost two years. My last question on our WhatsApp group from two years ago is still waiting an answer but that is not relevant now.
No, I think it should just be hardcoded part of the device type, see here: aioswitcher/src/aioswitcher/device/__init__.py Lines 38 to 46 in 6b477af Options:
I am getting confused again, didn't you just mention "Right now, Only Switcher Runner S11, Switcher Runner S12 and Switcher Lights require a Token" and "The only plan right now is to add a Token logic to the Switcher Runner (once again at least 1 year from now)."? This was the base assumption why we go forward without knowing the token logic, all of the above are Api2 devices, so how come we discuss Api1 again? I think you need to clear those things up. As for the token, if you go with option 1, make the token an optional parameter for Api2 (token: str | None = None) if you go with option 2 add a new required parameter. |
Beta Was this translation helpful? Give feedback.
-
|
Let's say you want to call set_position: aioswitcher/scripts/control_device.py Lines 442 to 452 in 6b477af How can you tell in this case if you need a token or not? it's just "SwitcherType2Api" calling it. You have to know on the API call if the device needs a token or not. So I propose 2 options:
|
Beta Was this translation helpful? Give feedback.
-
|
@thecode For option 1 its like this: aioswitcher/src/aioswitcher/device/tools.py Lines 145 to 165 in 755f964 |
Beta Was this translation helpful? Give feedback.
-
|
We discussed about adding a field that indicates if token is needed to the discovery messages, see #715 (comment) if you do that you know if you need a token or not. You can just add the token as optional parameter to the API and the caller will add it when needed |
Beta Was this translation helpful? Give feedback.
-
|
I would like a solution that works for HA and this library.. I don't want to break this library so that it will only work in HA.. |
Beta Was this translation helpful? Give feedback.
-
|
My suggestion in #715 (comment) would work for both options and will be the minimal effort from user side, as if token is needed he should know it and add it to the command line anyhow. |
Beta Was this translation helpful? Give feedback.
-
But then it would only work for HA.. This library would be broken.. |
Beta Was this translation helpful? Give feedback.
-
But you are talking about discovery time. This library can directly reach device operations without any discovery bridging.. it won't work here.. |
Beta Was this translation helpful? Give feedback.
-
You are missing the point. The discovery can tell you if you need a token or not, after that you don't need it anymore. Of course the library can do operations without discovery, but discovery needs to happen at least one time for a user to know the device details, than he can store and use them (without the need for discovery next time). The webapi users do the same, they run the discovery script, store the device id & ip and use it for their API calls (otherwise they need to guess the device ID somehow). |
Beta Was this translation helpful? Give feedback.
-
|
@thecode Now you want let's say to set the Runner position accessing this part eventually: https://github.com/TomerFi/aioswitcher/blob/dev/src/aioswitcher/api/__init__.py#L703-L736 We have 2 problems:
For those problems, I suggested the "know the device type" logic and also the send token with the API. This is what we need to have So regarding
|
Beta Was this translation helpful? Give feedback.
-
|
In this case lets go with your suggestion to add the device type to the API calls, but I please also add a hardcoded flag for each device in the discovery to indicate if it needs a token or not. |
Beta Was this translation helpful? Give feedback.
-
|
@thecode Sounds great so we go with PR (1) and I close the (2). I will add that flag in the discovery as you mentioned. |
Beta Was this translation helpful? Give feedback.
-
|
@thecode For the discovery here is an example: {
'device_id': '6442c6',
'device_key': '04',
'device_state': <DeviceState.ON: ('01', 'on')>,
'device_type': <DeviceType.RUNNER_S11: ('Switcher Runner S11', '0f01', 2, <DeviceCategory.SHUTTER_SINGLE_LIGHT_DUAL: 5>, True)>,
'direction': <ShutterDirection.SHUTTER_STOP: ('0000', 'stop')>,
'ip_address': '192.168.1.200',
'last_data_update': datetime.datetime(2024, 3, 24, 18, 4, 18, 992213),
'light': <LightState.OFF: ('00', 'off')>,
'light2': <LightState.OFF: ('00', 'off')>,
'mac_address': 'FH:41:3F:DA:26:3C',
'name': 'Switcher Runner_6CF5',
'position': 100,
'token_needed': True
}Is it ok that I added the "token_needed" field in the discovery? or we just use device_type.token_needed instead? Also, what is a better name for a function: "is_using_token" or "token_needed"? |
Beta Was this translation helpful? Give feedback.
-
|
@thecode @TomerFi The PR is based on this #744 |
Beta Was this translation helpful? Give feedback.
-
|
@thecode @thecode Me and the community will have to yet use the HomeBridge Switcher plugin to support those devices. |
Beta Was this translation helpful? Give feedback.
-
|
@TomerFi I hear you and I am really trying to do my best here.. The lack of direct communication is really hurts this progress. |
Beta Was this translation helpful? Give feedback.
-
|
What lack of direct communication are you referring to? We talk here, via emails, and on LinkedIn. Maybe I missed it, but I don't remember seeing any mention of Switcher giving you a "black box" code for your PR until you opened the PR and claimed we're uncooperative for asking questions about it. Please don't hesitate to share any ideas here to help improve the communication. Please pay attention. I asked you to close the drafts. This is like reviewing your PRs; you ignore reviews you disagree with. I now need to go to your new PR and verify all my change requests from the previous PR. You are making reviewing frustrating! Please make more effort on collaboration. I'll close the drafts; please do not merge the opened one until I see it. |
Beta Was this translation helpful? Give feedback.
-
|
I didn't ignore your request to close those PRs.. they were there for a reasonable reasons and I mentioned that.. #744 I believe should have been open so I can small merge stuff to into that and then merge that as a whole to dev branch, and the other one was so we can keep track of what is left of that after I split into smaller PRs.. But ok I will open them once needed.. |
Beta Was this translation helpful? Give feedback.
-
|
I don't understand the approach or its effectiveness. But whatever works for you... |
Beta Was this translation helpful? Give feedback.
-
|
On another note - I finished my review. Thank you for making this PR small and clear. |
Beta Was this translation helpful? Give feedback.
-
|
Yogev, please please bring the PR back! I know that like me there are many
other people anxiously waiting for this to happen already. Sure it took way
too long but we all saw how dedicated you were to this cause... This is a
really important feature, please reconsider and bring it back! Thank you so
much for all that you've done so far.
…On Fri, Apr 5, 2024, 01:48 Tomer Figenblat ***@***.***> wrote:
I'm sorry you feel that way. Please see this #747 (comment)
<#747 (comment)>
as an answer. If you change your mind and are willing to collaborate, I
still think we can make it work.
—
Reply to this email directly, view it on GitHub
<#715 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN25E3RUV7FFKZBKTYJ6QLY3XKDNAVCNFSM6AAAAAA5CHTQBKVHI2DSMVQWIX3LMV43SRDJONRXK43TNFXW4Q3PNVWWK3TUHM4TAMJVGE4DM>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
@moryoav |
Beta Was this translation helpful? Give feedback.
-
|
Work, research, time, and effort—all this, put into a contribution, is always welcomed and very much appreciated. Unfortunately, it doesn't correlate directly to the quality of the work or the acceptance of the contribution. |
Beta Was this translation helpful? Give feedback.
-
|
I am splitting #747 into smaller PRs starting with #764. |
Beta Was this translation helpful? Give feedback.
-
|
@YogevBokobza @thecode We missed CI workflows not running for PR opened against a branch other than dev. Fixed in #765. Let's merge this. |
Beta Was this translation helpful? Give feedback.
-
|
Cool :) |
Beta Was this translation helpful? Give feedback.
-
|
@YogevBokobza done. Please rebase and force push #764 against runner-support-fixed. |
Beta Was this translation helpful? Give feedback.
-
|
thank you @YogevBokobza for all your hard work!!! |
Beta Was this translation helpful? Give feedback.
-
|
@TomerFi |
Beta Was this translation helpful? Give feedback.
-
|
What is so important to rebase this branch? It has nothing which conflicts with the dev branch |
Beta Was this translation helpful? Give feedback.
-
|
Actually.. there are conflicts.. I tried to rebase but got stuck on the way.. |
Beta Was this translation helpful? Give feedback.
-
|
I will take a crack at this today. |
Beta Was this translation helpful? Give feedback.
-
|
@YogevBokobza @thecode Rebase done, please git pull --force the runner-support-fixed before continuing further. |
Beta Was this translation helpful? Give feedback.
-
|
@TomerFi @thecode The actual indexes used from those 2 functions are different between those 2 places and I will try to explain with an example.. So when controlling the device those are the outputs that need to be returned from the functions. But, for the discovery, it should take those indexes and subtract 1 from the returned functions. I have found 2 solutions for that:
NOTE: The updated code for controlling the device is not pushed yet and I am testing this locally with the actual device. Let me know what you think is better.. |
Beta Was this translation helpful? Give feedback.
-
|
Lets try to move forward and not revert things that already approved, it is hard for me to comment on code that I haven't seen yet, when you make the next PR we can see how we can improve the index operations. |
Beta Was this translation helpful? Give feedback.
-
|
Ok so I will bring the PR up and we discuss it then. |
Beta Was this translation helpful? Give feedback.
-
|
I am taking a week off for vacation.. I will get back after that. |
Beta Was this translation helpful? Give feedback.
-
Description
Based on an investigation performed by @YogevBokobza, new Runner firmware versions require the use of a token.
Problems
Currently, it looks like the said token can only be fetched manually by accessing the vendor's servers. No API is currently available for programmatic access.
Not all firmware versions require the use to token, and for now it appears we don't have an out-of-the-box approach for determining which ones do.
This project has 2 main clients, Home Assistant Integration Switcher KIS and a Containerized WebAPI Switcher WebAPI. For the latter, including vendor server access will force a change of the integration class.
Guidelines
This project works locally only, multiple people have worked really hard along the years to make it possible with the goal of making the underlying PyPi module more robust. We should strive to keep it that way.
Home Assistant should be onboard with any change, everything needs to be in the spirit of HA. Although not the prefered path, but if not accepted by HA, splitting the PyPi module into 2 will be considered.
A target branch was created for for any related work, any PR should be based from runner-support.
This is an open discussion, anyone is welcome to comment and contribute. That being said, both the topic of adding support for new devices, and the topic of changing the class of HA integration can become a bit heated, it's important keep this project's Code of Conduct in mind, and assume good intentions by all.
Project PRs
More information can be found in the following PRs:
Client PRs
Contacts
Thanks are in order for all current and future involved. All the time and hard work invested in this are very much appreciated.
Beta Was this translation helpful? Give feedback.
All reactions