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

Nissan Leaf Integration (Carwings / NissanConnect EV) #19786

Merged
merged 82 commits into from Feb 15, 2019

Conversation

@filcole
Copy link
Contributor

filcole commented Jan 5, 2019

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:

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

BenWoodford and others added some commits Jan 11, 2018

Merge in fixes made by Phil Cole
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

dgomes approved these changes Feb 11, 2019

Copy link
Member

dgomes left a comment

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

filcole and others added some commits Feb 11, 2019

fabaff added some commits Feb 15, 2019

@fabaff

fabaff approved these changes Feb 15, 2019

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

4 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Feb 15, 2019

@fabaff fabaff modified the milestone: 0.88.0 Feb 15, 2019

@klaasnicolaas

This comment has been minimized.

Copy link
Member

klaasnicolaas commented Feb 15, 2019

Release 0.88 or 0.89?

@stbenjam

This comment has been minimized.

Copy link

stbenjam commented Feb 15, 2019

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

@bachya

This comment has been minimized.

Copy link
Contributor

bachya commented Feb 15, 2019

@klaasnicolaas This has been marked for 0.88. 👇

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 15, 2019

No, this will go out in 0.89.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

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.


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

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 15, 2019

Member

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

Show resolved Hide resolved homeassistant/components/nissan_leaf/__init__.py
Show resolved Hide resolved homeassistant/components/nissan_leaf/__init__.py
try:
# This might need to be made async (somehow) causes
# homeassistant to be slow to start
await hass.async_add_job(leaf_login)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 15, 2019

Member

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.

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

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 15, 2019

Member

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.

Show resolved Hide resolved homeassistant/components/nissan_leaf/sensor.py
Show resolved Hide resolved homeassistant/components/nissan_leaf/switch.py
Show resolved Hide resolved homeassistant/components/nissan_leaf/switch.py
self.car.data[DATA_CLIMATE] = False

@property
def icon(self):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 15, 2019

Member

Remove this.

This comment has been minimized.

@filcole

filcole Feb 16, 2019

Author Contributor

Remove this.

I don't understand the change needed here

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 16, 2019

Member

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

if await self.car.async_start_charging():
self.car.data[DATA_CHARGING] = True

def turn_off(self, **kwargs):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 15, 2019

Member

Remove this if we don't support it.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 15, 2019

Member

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

This comment has been minimized.

@filcole

filcole Feb 17, 2019

Author Contributor

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

@filcole

This comment has been minimized.

Copy link
Contributor Author

filcole commented Feb 16, 2019

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

This comment has been minimized.

Copy link

BenWoodford commented Feb 16, 2019

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

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 16, 2019

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

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 16, 2019

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

@filcole filcole referenced this pull request Feb 17, 2019

Merged

Nissanleaf #21145

7 of 9 tasks complete

@balloob balloob referenced this pull request Mar 6, 2019

Merged

0.89.0 #21712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.