search

msp1974 / homeassistant-jlrincontrol

An integration for JLR InControl to Home Assistant
MIT License
65 stars 12 forks source link

Charging and Climate switches not working in 3.0.0 beta3 #125

Closed Liam-Whiteside closed 8 months ago

Liam-Whiteside commented 9 months ago

I'm getting problems trying some of the new switches.

Charging (I'm not quite sure what this is supposed to do, but...), when switching on:

Logger: homeassistant.components.websocket_api.http.connection
Source: components/websocket_api/commands.py:238
Integration: Home Assistant WebSocket API (documentation, issues)
First occurred: 14:13:05 (1 occurrences)
Last logged: 14:13:05

[140581630699328] 'NoneType' object is not iterable
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/websocket_api/commands.py", line 238, in handle_call_service
    response = await hass.services.async_call(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/core.py", line 2149, in async_call
    response_data = await coro
                    ^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/core.py", line 2186, in _execute_service
    return await target(service_call)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/entity_component.py", line 272, in handle_service
    return await service.entity_service_call(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 882, in entity_service_call
    single_response = await _handle_entity_call(
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 952, in _handle_entity_call
    result = await task
             ^^^^^^^^^^
  File "/config/custom_components/jlrincontrol/switch.py", line 81, in async_turn_on
    params = self.get_service_params(True)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/config/custom_components/jlrincontrol/switch.py", line 125, in get_service_params
    params = list(service.get("params"))
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'NoneType' object is not iterable

Climate : in the JLR app, I have the option of using either "Engine Climate" (which only works when the charge cable isn't plugged in) or "Electric Climate". If I try to use the new switch it doesn't appear to do anything (the car is currently plugged in, although I don't see any errors)

config_entry-jlrincontrol-3a5a766573d9cc96491cf9b72151bb8c.json.txt

Liam-Whiteside commented 9 months ago

I've found that if I disconnect the charger, climate does work to start the engine and run the air conditioning. However if the charger is connected, nothing happens, except I see a push notification to the JLR app on my phone saying climate has turned off.

ismarslomic commented 9 months ago

File "/config/custom_components/jlrincontrol/switch.py", line 125, in get_service_params params = list(service.get("params")) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ TypeError: 'NoneType' object is not iterable

This is a bug for Charging, since we are trying to get field params from SUPPORTED_SWITCH_SERVICES["CP"] which does not exist.

msp1974 commented 9 months ago

Yes, couple of issues here. The bug in charging as @ismarslomic says but also the fact that you have a PHEV and you can have both 'Climate' which is starting the engine but also 'Preconditioning' which is running from electric. We need to look at this as we currently only show 1 or the other, so in essence you are missing the preconditioning option for when you are plugged in (or it being smart enough to know which service to call on a PHEV!) - JLR do make this super complicated across different platforms!

ismarslomic commented 9 months ago

@Liam-Whiteside We have found a bug for the Preconditioning switch, which we have a fix for (at least when testing in Electric car (I Pace). We want also to understand what data we retrieve when the climate is activated in PHEV cars as well.

Could you please help us out with following?

We need to see which fields are updated with what values in the diagnostic file, so we can adjust the logic for PHEV cases as well

Liam-Whiteside commented 9 months ago

Ah I can see a preconditioning option as well - that does actually work. I had assumed the preconditioning would be the preconditioning of the battery (ie getting it up to temperature before a journey), rather than anything to do with the cabin air con. In the JLR app there's an option to set a vehicle departure time with "smart charging" which I think gets the battery ready to use at the specified time.

Liam-Whiteside commented 9 months ago

Here's the diags with the electronic climate running.

config_entry-jlrincontrol-3a5a766573d9cc96491cf9b72151bb8c.json (1).txt

Liam-Whiteside commented 9 months ago

During that last diags the car was charning, as sown in the app, however the Charging switch wasn't on in HA, which I'd have expected. image

ismarslomic commented 9 months ago

I had assumed the preconditioning would be the preconditioning of the battery (ie getting it up to temperature before a journey), rather than anything to do with the cabin air con.

I agree that the term Preconditioning can be confusing, specially when those owning PHEV have an Climate switch as well. For my car the JLR app is using the Climate term, and it is used to pre heat the air in the car, not the engine or battery.

Not sure if we could find an better name for Preconditioning, what do you think @msp1974?

image

ismarslomic commented 9 months ago

Here's the diags with the electronic climate running.

config_entry-jlrincontrol-3a5a766573d9cc96491cf9b72151bb8c.json (1).txt

Thanks, will take a look on it tomorrow. Could you please share the diags file when enabling climate with the engine as well. So we can check if there are any differences?

Liam-Whiteside commented 9 months ago

Need to go and physically unplug the car to get the other diags. The JLR app shows both options as Climate for the PHEV

image

ismarslomic commented 9 months ago

Thanks for the screenshot, that really helps in order to understand differences between the cars.

No hurry disconnecting the charging cable, do it when it fits you best!

Liam-Whiteside commented 9 months ago

Status shows "engine on remote start", but climate doesn't show as "on" even though it's running. It would be good if the switch and the sensor showed the current state of what the car is actually doing.

config_entry-jlrincontrol-3a5a766573d9cc96491cf9b72151bb8c.json (2).txt

ismarslomic commented 9 months ago

Coudn't wait to take a look on your diags file when the Electric Climate is running.

We have the same state in the same fields of diags file in my electric car and yours PHEV. You can actually start the Electric Climate in your car from HA by using the Preconditioning switch. The only issue is that the switch will first be set to ON, and then back to OFF. This is the know bug that we will fix (see #127). But if you check the state of Climate in JLR app, you will se that its running.

So the main issue here is to fix the Climate switch, which starts the Engine Climate (given car is unplugged), which most probably is not working from HA currently. But when we have the diags file for Engine Climate, we can fix it.

ismarslomic commented 9 months ago

but climate doesn't show as "on" even though it's running

This is known bug, Climate switch should be ON. Thanks for the diags file, very helpful! Will take a look on this tomorrow.

Are you comfortable pulling this integration from dev branch by use of git? Not sure how techy you are or want to be 😂

Liam-Whiteside commented 9 months ago

I was wondering if the API would show Climate and Preconditioning as separate things, even though in both cases the Air Con is running. I've ben downloading the beta releases, but happy to pull files from git to test (might need some pointers where to put files and which ones to pull). I'm running a supervised install of HA on Ubuntu 22.04, so slightly non standard but more access to tweak things.

ismarslomic commented 9 months ago

Ok, lets see if we maybe could just make new beta release, so we avoid pulling from dev branch manually.

The JLR API call these things Preconditioning and Climate, but in the UI (JLR app) they name them:

Climate: Engine Climate (for PHEV) Preconditioning: Electric Climate (for PHEV) and Climate (for electric cars)

Maybe we should do the same in HA? At least that would be consistent with the JLR app, and less confusing for the users not having any experience with the API.

ismarslomic commented 9 months ago

@msp1974 would it be possible to switch the default branch from master to dev in this Github repo? That will make it easier for user to help testing fixes in 3.0.0 directly from HACS. Or do you prefer to create beta releases instead?

Liam-Whiteside commented 8 months ago

Having updated HA i've just got the latest files from the dev branch. Confirmed the new Climate (Electric) and Climate (Engine) are working as expected (DS hybrid)

Liam-Whiteside commented 8 months ago

Charging switch still not working, giving this error

Logger: homeassistant.components.websocket_api.http.connection Source: components/websocket_api/commands.py:240 Integration: Home Assistant WebSocket API (documentation, issues) First occurred: 08:59:58 (1 occurrences) Last logged: 08:59:58

[140529013579968] 'NoneType' object is not iterable Traceback (most recent call last): File "/usr/src/homeassistant/homeassistant/components/websocket_api/commands.py", line 240, in handle_call_service response = await hass.services.async_call( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/core.py", line 2279, in async_call response_data = await coro ^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/core.py", line 2316, in _execute_service return await target(service_call) ^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 892, in entity_service_call single_response = await _handle_entity_call( ^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 962, in _handle_entity_call result = await task ^^^^^^^^^^ File "/config/custom_components/jlrincontrol/switch.py", line 81, in async_turn_on params = self.get_service_params(True) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/config/custom_components/jlrincontrol/switch.py", line 125, in get_service_params params = list(service.get("params")) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ TypeError: 'NoneType' object is not iterable

ismarslomic commented 8 months ago

Thanks for the feedback, @Liam-Whiteside .

I have pushed fix for charging in dev branch. Could you please try now and test if it works?

Liam-Whiteside commented 8 months ago

I still seem to be getting an error (the car isn't currently plugged in). Do you know what this switch should actually do? (currently I'm just expecting to not get an error from the integration, but ideally I'd like to be able to enable / disable charging so that unless I really needed it, the car wouldn't charge during peak energy times)

Logger: homeassistant.components.websocket_api.http.connection Source: components/websocket_api/commands.py:240 Integration: Home Assistant WebSocket API (documentation, issues) First occurred: 10:51:51 (1 occurrences) Last logged: 10:51:51

[140228398647232] string indices must be integers, not 'str' Traceback (most recent call last): File "/usr/src/homeassistant/homeassistant/components/websocket_api/commands.py", line 240, in handle_call_service response = await hass.services.async_call( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/core.py", line 2279, in async_call response_data = await coro ^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/core.py", line 2316, in _execute_service return await target(service_call) ^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 892, in entity_service_call single_response = await _handle_entity_call( ^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 962, in _handle_entity_call result = await task ^^^^^^^^^^ File "/config/custom_components/jlrincontrol/switch.py", line 84, in async_turn_on result = await jlr_service.async_call_service(**params) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/config/custom_components/jlrincontrol/services.py", line 140, in async_call_service success = await self.async_monitor_service_call( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/config/custom_components/jlrincontrol/services.py", line 194, in async_monitor_service_call status["vehicleId"] = field_mask(status["vehicleId"], 3, 2)


TypeError: string indices must be integers, not 'str'
msp1974 commented 8 months ago

@Liam-Whiteside , I think we need to turn on debugging to see what is getting returned from the service call. It is clearly failing but when trying to log the error is not getting the info expected.

When you switch this button, you should get messages showing the service call and the updates coming back from the jlr servers. We need to see these.

BTW, this is supposed to be the charge start/stop function that is (or used to be) in the app.

Liam-Whiteside commented 8 months ago

Ok, I assume I "enable debug logging" in the integration... does it then need a HA restart? Where does it generate data?

Happy to use curl and make manual calls to my car if that's more help.

ismarslomic commented 8 months ago

@Liam-Whiteside I assume that you dont use dev-branch of the integration, because the error you are getting has been fixed in #122. To make it easier for you and others I will publish new beta release in few minutes, and let you know when you can get latest code. Then I would appreciate help from you testing few things specifically for PHEV.

Liam-Whiteside commented 8 months ago

After the fix for HA Core 2024.02 I manually got the updated const.py and coordinator.py this morning from https://github.com/msp1974/homeassistant-jlrincontrol/raw/dev/custom_components/jlrincontrol/ Prior to that I was running beta3, so I may not have got all the updates.

Liam-Whiteside commented 8 months ago

I've now refreshed all files from the dev branch and not seeing any errors now. Will test "Charging" again once the car is plugged in.

ismarslomic commented 8 months ago

New beta release v3.0.0beta4 is ready. Please replace local changes with this release before testing further, so we avoid any confusion about local vs remote changes.

ismarslomic commented 8 months ago

@Liam-Whiteside could you please help out on testing following, so we can verify if issues with climate (engine) and charging you have reported in this issue has been resolved:

  1. Climate (engine): toggle the Climate (engine) switch to ON and verify that the state of the Climate sensor shows value On after few seconds / minutes
  2. Charging: plug charging cable to the car and toggle the Charging switch to ON and OFF and verify in the JLR app that car is charging / not charging
Liam-Whiteside commented 8 months ago

On Thu, 8 Feb 2024 at 13:15, Ismar Slomic @.***> wrote:

@Liam-Whiteside https://github.com/Liam-Whiteside could you please help out on testing following, so we can verify if issues with climate (engine) and charging you have reported in this issue has been resolved:

  1. Climate (engine): toggle the Climate (engine) switch to ON and verify that the state of the Climate sensor shows value On after few seconds / minutes

Works correctly. Climate sensor changed to on and status changed to "Engine On Remote Start". changed back shortly after switching off.

  1. Charging: plug charging cable to the car and toggle the Charging switch to ON and OFF and verify in the JLR app that car is charging / not charging

Plugged the charging cable in, charge switch changed to On and battery sensor updated to show charging. Turned the switch off, battery sensor changed to charging state paused. Turned the charging switch on, charging resumed.

All changes reflected between HA and the JLR app.

So all looks good. I think this ticket can be closed.

ismarslomic commented 8 months ago

Thanks @Liam-Whiteside