Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User input for Pin code required in configuration AND services in 3.0.0 Beta 2 #124

Open
ismarslomic opened this issue Feb 3, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@ismarslomic
Copy link
Collaborator

@msp1974 The Pin code is required in the integration configuration and most of the service calls. Couldn't we just require the Pin code in the configuration and read it from configuration properties for each service call?

@msp1974
Copy link
Owner

msp1974 commented Feb 4, 2024

Yes, this probably makes sense.

@ismarslomic
Copy link
Collaborator Author

More inputs on Pin code:

  1. we should probably not set any default value(0000) for Pin code in the config flow. This is required field and there is really none default value for this field
  2. Im not sure if Pin code 0000 is actually accepted or not when configured in the car. However, we shouldn't check and assume that this is not accepted, like we do here.

@msp1974
Copy link
Owner

msp1974 commented Feb 4, 2024

In principle agree, however we have to migrate from the previous optional config. I wonder if we set a default of 0000 in the migration if not already set but then remove pin requirement in services and raise error if not set when calling them.

@msp1974 msp1974 added the enhancement New feature or request label Mar 9, 2024
msp1974 added a commit that referenced this issue Apr 22, 2024
…r Pin code required in configuration AND services in 3.0.0 Beta 2 #124
@msp1974
Copy link
Owner

msp1974 commented Apr 22, 2024

OK, when looking at this, there is a slight issue if you have more than one car on the account with different pins. Therefore made the pin in the service call optional. So it should work like this:

  1. When migrating from v2.2.7, the pin is set to 0000 if not set in options.
  2. In v3.0.0, if pin not changed from 0000, it will error if pin not provided in service call
  3. In v3.0.0, if pin is set to something other than 0000 in config and not provided in service call, will pass config pin
  4. In v3.0.0, if pin is set to something other than 0000 in config but supplied in service call, will use service call pin (to allow for the multi vehicle scenario).

Do you think this is good logic to support migration of users not having set in options on older verison and also the multi vehicle on 1 account scenarios. Will also ensure those migrating with service calls in automations will continue to work.

@ismarslomic
Copy link
Collaborator Author

ismarslomic commented Apr 22, 2024

Hi Mark! Hope you enjoyed time off from computer for the past 6 weeks :)

Good point regarding multiple cars, I focused only on my own use case having one car. Before commenting on your suggestions, how does the integration (v3) handle multiple cars in HA? You setup the JLR integration with a Pin code and then you get one device per vehicle sharing the same Pin code. Correct?

If so, how does the new Switch and Button (Charging, Climate, Guardian Mode, Climate, Service Mode etc) entities behave when you configure only one Pin Code in integration configuration flow? Service calls are just an alternative to the new Switch and Button entities automatically created for a device.

@msp1974
Copy link
Owner

msp1974 commented Apr 22, 2024

Hmmmm...that's a very good point. Need to think about that and find a solution. But yes, one device per vehicle with same set of entities.

@ismarslomic
Copy link
Collaborator Author

Would it be possible to set restriction: one vehicle per integration entry? So if you have two vehicles you define two separate integration entries, with their own pin code and one single device? Then we could maybe read the pin code corresponding to the target entity / device in service calls?

image
image

@msp1974
Copy link
Owner

msp1974 commented Apr 22, 2024

Maybe. To do that we would have to read the vehicles during setup in the config flow, so I was thinking if we have to do that anyway, we could ask for a pin for each vehicle during config flow. I'll have a play this week and see which option works best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants