Nissan Leaf Integration (Carwings / NissanConnect EV) by filcole · Pull Request #19786 · home-assistant/core

filcole · 2019-01-05T07:46:10Z

Description:

Adds support for monitoring and controlling the Nissan Leaf remote control platform.

Currently introduces two switches (climate control and charging, charging can only be turned on as per the API limitation), 3 sensors (battery charge and range with and without AC on), a binary sensor for if the car is currently plugged in, device tracker for the car location (if the car is capable).

I'm based in the UK with a 2016 30kWh leaf so assistance from others is very useful.

Example entry for configuration.yaml:

nissan_leaf:
  username: "username"
  password: "password"
  region: 'NE'     # Indicates Europe, must be changed for other regions
  nissan_connect: true  # Set to false for early Leafs 24kWh that don't report location
  update_interval: 
    hours: 1
  update_interval_charging: 
    minutes: 15
  update_interval_climate: 
    minutes: 5
  force_miles: true

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

home-assistant/home-assistant.io#8073

Remove invalid FIXMEs and update TODOs Fixes for pylint and test for CarwingsError exception rather than Exception Flake8 fixes Add pycarwings2 to requirements_all.txt Add extra configuration documentation. Use pycarwings2 from pip. Check server dates between requests. Add sensor device class for battery. Async conversion fixes flake8 fixes and docstrings Non-async charging is OK Handle multiple cars in the configuration Convert to async. Better imports for platforms Fix scanning interval & prevent extra refreshes. async switchover Check discovery_info to prevent load of platforms Ensure update frequency is always above a minimum interval (1 min). Platforms don't have return values Use values() instead of items() when not using key Use snake_case (LeafCore becomes leaf_core) commit 418b6bb

dgomes

I'll leave to other reviewers to have a second go :)

klaasnicolaas · 2019-02-15T14:06:40Z

Release 0.88 or 0.89?

stbenjam · 2019-02-15T14:55:55Z

Thank you so much @filcole and @BenWoodford!!!! And all the reviewers 👍 Can't wait for the next release of HA

bachya · 2019-02-15T18:35:22Z

@klaasnicolaas ~~This has been marked for 0.88.~~ 👇

MartinHjelmare · 2019-02-15T18:38:32Z

No, this will go out in 0.89.

MartinHjelmare

Please open a new PR where we can address the comments.

Generally I think there is too much logging and the update logic seems complicated.

Consider use of async vs sync api. Don't do I/O in event loop.

I'm looking forward to the next PR.

MartinHjelmare · 2019-02-15T18:42:52Z

+
+        if vin in hass.data[DATA_LEAF]:
+            data_store = hass.data[DATA_LEAF][vin]
+            async_track_point_in_utc_time(


Why use the listener if we need to call the action now?

MartinHjelmare · 2019-02-15T18:48:06Z

+        try:
+            # This might need to be made async (somehow) causes
+            # homeassistant to be slow to start
+            await hass.async_add_job(leaf_login)


hass.async_add_job is legacy. Use hass.async_create_task for coroutines if we want to track the task and wait for completion before finishing setup phase. Otherwise use hass.loop.create_task if we don't want to wait for completion during setup phase.

MartinHjelmare · 2019-02-15T18:49:00Z

+        async def leaf_login():
+            nonlocal leaf
+            sess = pycarwings2.Session(username, password, region)
+            leaf = sess.get_leaf()


We're not allowed to do I/O in event loop ie in coroutines. This needs to move to be scheduled on the executor thread pool. From async context that's done with hass.async_add_executor_job.

Any reason why we want to use the home assistant async api when the client library doesn't support asyncio? Usually it's easier to use our sync api if the library is sync.

MartinHjelmare · 2019-02-15T19:24:48Z

+            self.car.data[DATA_CLIMATE] = False
+
+    @property
+    def icon(self):


Remove this.

Remove this.

I don't understand the change needed here

The switch entity already has frontend representation tailored to that entity. We usually don't overwrite this property for switches.

MartinHjelmare · 2019-02-15T19:25:23Z

+        if await self.car.async_start_charging():
+            self.car.data[DATA_CHARGING] = True
+
+    def turn_off(self, **kwargs):


Remove this if we don't support it.

Maybe this shouldn't be a switch at all? Maybe a scene is better?

Yes, switch is sub-optimal. Perhaps sensor and service - discuss in new pull request.

filcole · 2019-02-16T07:54:44Z

Please open a new PR where we can address the comments.

Generally I think there is too much logging and the update logic seems complicated.

Consider use of async vs sync api. Don't do I/O in event loop.

I'm looking forward to the next PR.

@MartinHjelmare Thank you for the review, and also to @fabaff and @dgomes. I will submit another pull request when I've tried to address the issues raised. I hope you don't mind if I comment against the conversation points raised as I'm attempting to resolve them.

BenWoodford · 2019-02-16T18:49:51Z

When you say too much logging, do you mean logging using info or in general?

Part of the reason for so much logging is that Nissan break their API on a semi-regular basis, and sometimes only for certain people (like how they only broke battery charge for older cars that report in bars). They have basically no sense of DevOps for API versioning/maintenance so the logging is needed at least on trace to debug issues that only occur for certain people.

MartinHjelmare · 2019-02-16T19:38:11Z

Both. Generally we don't want to log on info level in components. If it's not a warning or error, we should use debug.

There are some more places where the debug logs only seem to be used to verify our own logic. That's not how we should use logging. If we need to verify our own logic, we should use tests.

It's ok to log the result of an api but we should probably not log that we called the api, and definitely not log internal component variable changes. Sounds good?

MartinHjelmare · 2019-02-16T19:40:14Z

For more discussion I would prefer to have that in the new PR. It's ok to open that before it's completely ready.

Ben Woodford and others added 21 commits February 1, 2018 14:33

Added work so far.

fb561dd

Change interval so nobody drains their battery when I put this online

7f80bd0

Added the warning notice.

18f91f8

Async setup

7080aac

Still broken, but we're getting there.

3aa168b

Back to synchronous, moved refresh stuff into DataStore

aa50030

Functional sensors!

b01060f

Added working switches, tweaked intervals a bit

d360846

Fixed turn off result

2d60163

Moved plug status to binary_sensor, added smart intervals

ad54f37

Documentation and car nickname stuff

3e20bd2

Syntax fixes and coveragerc additions

0a64cae

Style fixes

732fbe4

Fixing the final line length

e446aa4

Fixed an issue with newer models and bad climate data

053c383

Forgot to check my line endings.

a5d698d

New icons for most of the components

170fa1a

Hotfix for handling Nissan's awful servers

1693701

Merge in work completed by BenWoodford

cb46040

Merge branch 'BenWoodford-nissanleaf' into nissanleaf

231245b

homeassistant added integration: device_tracker integration: nissan_leaf core new-platform platform: binary_sensor.nissan_leaf cla-signed labels Jan 5, 2019

ghost added the in progress label Jan 5, 2019

Flake8 line fixes

fe68763

dgomes approved these changes Feb 11, 2019

View reviewed changes

Comment thread homeassistant/components/nissan_leaf/__init__.py

filcole and others added 4 commits February 11, 2019 22:43

Remove excess and double logging.

c4ced4b

Add updated_on attribute for device tracker.

6f5c00a

Fix crash if pycarwings2 doesn't provide cruising ranges.

85218d8

Minor changes

71c80dc

ghost assigned fabaff Feb 15, 2019

fabaff added 4 commits February 15, 2019 12:56

Minor changes

98229d2

Minor changes

2ceb788

Minor changes

db5d0b6

Minor changes

5d79e48

fabaff approved these changes Feb 15, 2019

View reviewed changes

fabaff merged commit 656d39e into home-assistant:dev Feb 15, 2019

ghost removed the in progress label Feb 15, 2019

fabaff modified the milestone: 0.88.0 Feb 15, 2019

MartinHjelmare reviewed Feb 15, 2019

View reviewed changes

filcole mentioned this pull request Feb 17, 2019

Nissanleaf #21145

Merged

9 tasks

balloob mentioned this pull request Mar 6, 2019

0.89.0 #21712

Merged

ghost removed platform: binary_sensor.nissan_leaf labels Mar 21, 2019

Uh oh!

Conversation

filcole commented Jan 5, 2019 • edited by fabaff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

dgomes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

klaasnicolaas commented Feb 15, 2019

Uh oh!

stbenjam commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bachya commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinHjelmare commented Feb 15, 2019

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filcole commented Feb 16, 2019

Uh oh!

BenWoodford commented Feb 16, 2019

Uh oh!

MartinHjelmare commented Feb 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinHjelmare commented Feb 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

filcole commented Jan 5, 2019 •

edited by fabaff

Loading

stbenjam commented Feb 15, 2019 •

edited

Loading

bachya commented Feb 15, 2019 •

edited

Loading

MartinHjelmare Feb 15, 2019 •

edited

Loading

MartinHjelmare commented Feb 16, 2019 •

edited

Loading