Nissan Leaf (Carwings/Nissan Connect EV) Platform#12116
Nissan Leaf (Carwings/Nissan Connect EV) Platform#12116BenWoodford wants to merge 17 commits intohome-assistant:devfrom
Conversation
|
Hi @BenWoodford, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
|
|
||
| def turn_off(self, **kwargs): | ||
| _LOGGER.debug( | ||
| "Cannot turn off Leaf charging - Nissan does not support that remotely.") |
|
|
||
| @property | ||
| def is_on(self): | ||
| return self.car.data[LeafCore.DATA_CHARGING] == True |
There was a problem hiding this comment.
comparison to True should be 'if cond is True:' or 'if cond:'
|
|
||
| def log_registration(self): | ||
| _LOGGER.debug( | ||
| "Registered LeafChargeSwitch component with HASS for VIN " + self.car.leaf.vin) |
|
|
||
| @property | ||
| def is_on(self): | ||
| return self.car.data[LeafCore.DATA_CLIMATE] == True |
There was a problem hiding this comment.
comparison to True should be 'if cond is True:' or 'if cond:'
|
|
||
| def log_registration(self): | ||
| _LOGGER.debug( | ||
| "Registered LeafClimateSwitch component with HASS for VIN " + self.car.leaf.vin) |
|
|
||
| """ | ||
|
|
||
| self.signal_components() |
There was a problem hiding this comment.
IndentationError: unexpected indent
unexpected indentation
|
|
||
|
|
||
| """ | ||
| # Removing this block for now, as I do not have a Leaf with Nissan Connect to test it with. |
| self.data[DATA_CHARGING] = batteryResponse.is_charging | ||
| self.data[DATA_PLUGGED_IN] = batteryResponse.is_connected | ||
| self.data[DATA_RANGE_AC] = batteryResponse.cruising_range_ac_on_km | ||
| self.data[DATA_RANGE_AC_OFF] = batteryResponse.cruising_range_ac_off_km |
| " as climate control is on and the interval has passed.") | ||
| result = True | ||
|
|
||
| if result == True: |
There was a problem hiding this comment.
comparison to True should be 'if cond is True:' or 'if cond:'
| result = True | ||
| elif self.data[DATA_CLIMATE] == True and self.lastCheck + timedelta(minutes=self.config[DOMAIN][CONF_CLIMATE_INTERVAL]) < now: | ||
| _LOGGER.debug("Firing Refresh on " + self.leaf.vin + | ||
| " as climate control is on and the interval has passed.") |
|
Hi @BenWoodford, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
|
|
||
| @property | ||
| def unit_of_measurement(self): | ||
| if self.car.hass.config.units.is_metric is False or self.car.config[LeafCore.DOMAIN][LeafCore.CONF_FORCE_MILES] is True: |
There was a problem hiding this comment.
line too long (128 > 79 characters)
| else: | ||
| ret = self.car.data[LeafCore.DATA_RANGE_AC_OFF] | ||
|
|
||
| if self.car.hass.config.units.is_metric is False or self.car.config[LeafCore.DOMAIN][LeafCore.CONF_FORCE_MILES] is True: |
There was a problem hiding this comment.
line too long (128 > 79 characters)
|
|
||
| def log_registration(self): | ||
| _LOGGER.debug( | ||
| "Registered LeafRangeSensor component with HASS for VIN " + self.car.leaf.vin) |
|
|
||
| def log_registration(self): | ||
| _LOGGER.debug( | ||
| "Registered LeafBatterySensor component with HASS for VIN " + self.car.leaf.vin) |
| from .. import nissan_leaf as LeafCore | ||
| from ..nissan_leaf import LeafEntity | ||
| from homeassistant.helpers.entity import Entity | ||
| from homeassistant.helpers.dispatcher import ( |
There was a problem hiding this comment.
'homeassistant.helpers.dispatcher.async_dispatcher_connect' imported but unused
'homeassistant.helpers.dispatcher.dispatcher_send' imported but unused
|
|
||
| import logging | ||
| from .. import nissan_leaf as LeafCore | ||
| from ..nissan_leaf import LeafEntity |
There was a problem hiding this comment.
'..nissan_leaf.LeafEntity' imported but unused
| """ | ||
| Battery Charge and Range Support for the Nissan Leaf Carwings/Nissan Connect API. | ||
|
|
||
| Documentation pending, please refer to the main platform component for configuration details |
| @@ -0,0 +1,88 @@ | |||
| """ | |||
| Battery Charge and Range Support for the Nissan Leaf Carwings/Nissan Connect API. | |||
|
|
||
| def _update_callback(self): | ||
| """Callback update method.""" | ||
| #_LOGGER.debug("Got dispatcher update from Leaf platform") |
There was a problem hiding this comment.
block comment should start with '# '
| _LOGGER.debug("Empty Location Response Received") | ||
| self.data[DATA_LOCATION] = None | ||
| else: | ||
| LOGGER.debug("Got location data for Leaf") |
| self.data[DATA_PLUGGED_IN] = False | ||
| self.last_check = None | ||
| track_time_interval( | ||
| hass, self.refresh_leaf_if_necessary, timedelta(seconds=CHECK_INTERVAL)) |
|
Success! I got it installed onto my dev system. It appears to update sensors and just fine. I can turn on charging and climate control. I did not see the part about the tracker platform not being complete yet. I added it as a tracker to the config file anyway. The only bug I see is the Leaf’s charge is listed as 2000% at 100% charge. While I would like that upgrade, I remember reading on a forum that the bigger batteries report in a different format. I will see if I can find that. Also as note for your docs. I believe I read that the Nissan Leaf does not charge the 12 volt battery while it is plugged into a charger. It charges it from the main battery while driving and while parked unplugged. It also trickle charges the 12v from the solar panel if you have one. This would be a good question for the Nissan Leaf forums. For those like me who are noobs with doing github branches here is what I did to get my development raspberry pi with hassbian to install this branch. Please don’t try this on a production install, I am a noob at this. ssh into your Raspberry Pi ssh -l pi $ sudo systemctl stop home-assistant@homeassistant.service
|
There was a problem hiding this comment.
indentation contains tabs
no newline at end of file
There was a problem hiding this comment.
indentation contains tabs
continuation line under-indented for visual indent
There was a problem hiding this comment.
indentation contains tabs
indentation contains mixed spaces and tabs
continuation line under-indented for visual indent
There was a problem hiding this comment.
indentation contains tabs
missing whitespace after ','
There was a problem hiding this comment.
indentation contains tabs
no newline at end of file
There was a problem hiding this comment.
indentation contains tabs
continuation line under-indented for visual indent
There was a problem hiding this comment.
indentation contains tabs
indentation contains mixed spaces and tabs
continuation line under-indented for visual indent
There was a problem hiding this comment.
indentation contains tabs
missing whitespace after ','
There was a problem hiding this comment.
line too long (93 > 79 characters)
BenWoodford
force-pushed
the
nissanleaf
branch
from
e11599f to
170fa1a
Compare
|
|
||
| import logging | ||
|
|
||
| from .. import nissan_leaf as LeafCore |
There was a problem hiding this comment.
Python uses snake_case for all variable name that are not classes or root level variables
|
|
||
| _LOGGER.debug("Adding sensors") | ||
|
|
||
| for key, value in hass.data[LeafCore.DATA_LEAF].items(): |
There was a problem hiding this comment.
You're not using key, so use values() instead of items()
There was a problem hiding this comment.
As you can tell, Python is not my first language :)
|
|
||
| add_devices(devices, True) | ||
|
|
||
| return True |
There was a problem hiding this comment.
Platforms don't have return values
| # So using my fork for now. | ||
| # https://github.com/jdhorne/pycarwings2/pull/28 | ||
|
|
||
| REQUIREMENTS = ['https://github.com/BenWoodford/pycarwings2/archive/master.zip' |
There was a problem hiding this comment.
We should not merge this until we can point it at the updated pycarwings2 lib on PyPi
There was a problem hiding this comment.
Noted. I'm still waiting on the author to merge the PR in - something about Python 2 support.
| vol.Required(CONF_PASSWORD): cv.string, | ||
| vol.Required(CONF_REGION): vol.In(CONF_VALID_REGIONS), | ||
| vol.Optional(CONF_NCONNECT, default=True): cv.boolean, | ||
| vol.Optional(CONF_INTERVAL, default=DEFAULT_INTERVAL): cv.positive_int, |
There was a problem hiding this comment.
Validate intervals with vol.All(cv.time_period, cv.positive_timedelta), it will allow people to put in numbers but also "01:00". The validator converts all these different inputs to timedelta. You can get the value of the timedelta with value.total_seconds().
There was a problem hiding this comment.
Why would we actually let anyone override the intervals? As developers we should pick the right interval and not bother our users with it. Especially we don't want to allow the users to be able to DDOS the vendor and us getting the blame.
There was a problem hiding this comment.
I also just saw the warning about emptying the 12v battery, yep definitely we should not allow users to pick their own interval. We should pick conservative intervals too.
There was a problem hiding this comment.
I've given it very conservative intervals but left it open to people making their own if they have it plugged in more. Alternatively we could look at decreasing the rate of polling if the drive train battery is on a low charge as that's when the 12V is more at risk (the drive train battery will top up the 12V intermittently)
|
|
||
| _LOGGER.info("Successfully logged in and fetched Leaf info") | ||
| _LOGGER.info( | ||
| "WARNING: This may poll your Leaf too often, and drain the 12V." |
There was a problem hiding this comment.
We should not allow people to brick their car.
There was a problem hiding this comment.
This is mostly if they've left the car idle and on a low charge for a very long time. No helping that but it's a good warning to have.
|
|
||
| def setup_platform(hass, config, add_devices, discovery_info=None): | ||
| devices = [] | ||
|
|
There was a problem hiding this comment.
You will have to add
if discovery_info is not None:
returnto all platforms because the component is triggering a load of the platforms. Otherwise you will get issues when a user would add the leaf as a binary sensor platform.
| 'homebridge_model': 'Leaf' | ||
| } | ||
|
|
||
| @asyncio.coroutine |
There was a problem hiding this comment.
We've migrated to async/await syntax. Please drop this decorator and prefix async methods with async like async def async_added_to_hass . Replace any yield from with await.
| battery_response = self.get_battery() | ||
| _LOGGER.debug("Got battery data for Leaf") | ||
|
|
||
| if battery_response.answer['status'] == 200: |
There was a problem hiding this comment.
Why don't you store the battery_response object in the data store? That way you don't have to juggle around with all the DATA_ constants.
There was a problem hiding this comment.
How do you mean? I have lots of different responses from the car which is why I've got the constants.
There was a problem hiding this comment.
You can just store the latest response of every type
There was a problem hiding this comment.
OH, I see what you mean. Yeah that does make sense
| battery_status = self.leaf.get_status_from_update(request) | ||
| while battery_status is None: | ||
| _LOGGER.debug("Battery data not in yet.") | ||
| time.sleep(5) |
There was a problem hiding this comment.
Don't use sleep inside sync contexts as it blocks threads. It's ok to sleep within an async context, however, it seems weird to keep polling indefinitely? It should give up after a few times to make we don't break the 12v battery…
async def async_get_battery(self):
request = await self.hass.async_add_job(self.leaf.request_update)
for i in range(5):
if i > 0:
await asyncio.sleep(5)
battery_status = await self.hass.async_add_job(self.leaf.get_status_from_update, request)
if battery_status is not None:
break
return battery_statusThere was a problem hiding this comment.
I had been using async but was told that will block the main thread instead. Nissan's servers are really bad so they can be pretty sluggish sometimes.
The reason we sleep here is because the initial request will ask for the battery from the car, the requests in the while loop are then asking Nissan's servers if the car has updated them with the latest battery data. It's a pretty poor implementation by them - Tesla constantly phone home so you don't have these issues but Nissan's systems were coded by monkeys.
There was a problem hiding this comment.
It's ok to use async as long as you offload I/O to the executor (a thread worker pool). That is done in my example by passing the methods that do I/O (like request_update) to hass.async_add_job and awaiting that. Once the function is executed, the async context will resume.
balloob
left a comment
balloob
left a comment
There was a problem hiding this comment.
This PR is off to a great start! 👍
Left some comments, but also please:
Please rebase your branch on the latest dev as we no longer support Python 3.4. This should help with testing.
Travis reports a ton of linting issues, these need to be addressed
And let's get rid of configuring intervals so that people don't brick their cars.
|
Hi, I had this working the other day, even set up 'heat car' from google assistant (IFTTT), but now it's not working anymore and I get this in the logfile. Any help is appreciated, thanks. |
|
How can install this as a custom component? |
|
Hi @pblgomez , I think this thread is more for working on the plug-in, however, there is a thread on HomeAssistant forum. Here is a link that should help you get installed. Go to the bottom, there's a post from me. https://community.home-assistant.io/t/nissan-leaf-component-s-platform/38663 Odd, that link goes elsewhere :/ Copy the one below https://community.home-assistant.io/t/nissan-leaf-component-s-platform/38663 |
|
This PR seems to have gone stale. Closing it. You can reopen it when you're ready to finish it. |
ghost
removed
9 participants
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 weird API limitation), 3 sensors (battery charge and range with and without AC on) and a binary sensor for if the car is currently plugged in.
Further work needed at some point to add the device tracker, I have a 2015 Leaf so do not have this functionality on my car, right now the
nissan_connectconfig option does not do anything as it is used to enable the device tracker.Pull request in home-assistant.github.io with documentation (if applicable): documentation pending
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
Pending docs.
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable ([example][ex-requir]).requirements_all.txtby runningscript/gen_requirements_all.py.Currently using a GitHub URL, as the library's author is MIA and I had to fix up the library to get it working in HASS. Checked with @armills before submitting and have added a note to the code with the PR on
pycarwings2(jdhorne/pycarwings2#28).coveragerc.