Repository: bimmerconnected/bimmer_connected
Pull request #127 (2019-10-19 19:37:51)

Add ability to retry comms on HTTP 500 error
I find that (at least in my region), the Connected Drive servers can be pretty flaky sometimes and return a 500 error which will work with a retry 99% of the time.

This pull request adds the ability to configure this library to automatically retry HTTP 500 errors for the specified number of retries - although defaults to not doing this to preserve existing behavior.

Originally I'd done this in my code, but it's more reliable to do it in your library (if you're willing to accept this PR), as you have access to the HTTP response code. Otherwise I have to parse your error message and hope it stays the same or similar!

timmccor: I don't believe any of the remaining Travis errors are caused by my changes - looking at the Travis history it appears these tests usually fail. The remaning lint issues are not in the code I changed.

Please let me know if there's anything you'd like me to change.

Patch:
diff --git a/.github/ISSUE_TEMPLATE/issue-template.md b/.github/ISSUE_TEMPLATE/issue-template.md
index ee456e7..964a29f 100644
--- a/.github/ISSUE_TEMPLATE/issue-template.md
+++ b/.github/ISSUE_TEMPLATE/issue-template.md
@@ -14,7 +14,7 @@ A clear and concise description of what the issue is.
 A clear and concise description of what you expected to happen.
 
 **Home Assistant version**
-Which Home Assistant version are you using
+Which Home Assistant version are you using. And did it work on a previous version, if so what was the last working version.
 
 **Screenshots**
 If applicable, add screenshots to help explain your problem.
diff --git a/bimmer_connected/account.py b/bimmer_connected/account.py
index eadd6b3..1503cd8 100644
--- a/bimmer_connected/account.py
+++ b/bimmer_connected/account.py
@@ -14,6 +14,7 @@ import urllib
 import os
 import json
 from threading import Lock
+from time import sleep
 from typing import Callable, List
 import requests
 
@@ -35,10 +36,17 @@ class ConnectedDriveAccount:  # pylint: disable=too-many-instance-attributes
     :param log_responses: If log_responses is set, all responses from the server will
                 be loged into this directory. This can be used for later analysis of the different
                 responses for different vehicles.
+    :param retries_on_500_error: If retries_on_500_error is set, a communication with the
+                Connected Drive server will automatically be retried the number of times
+                specified in the event the error code received was 500. This sometimes
+                occurs (presumably) due to bugs in the server implementation.
+    :param retry_delay_on_500_error: The number of seconds to wait between retries, only takes
+                affect when retries_on_500_error is set to a number greater than 0.
     """
 
     # pylint: disable=too-many-arguments
-    def __init__(self, username: str, password: str, region: Regions, log_responses: str = None) -> None:
+    def __init__(self, username: str, password: str, region: Regions, log_responses: str = None,
+                 retries_on_500_error: int = 5, retry_delay_on_500_error: int = 1) -> None:
         self._region = region
         self._server_url = None
         self._username = username
@@ -47,6 +55,8 @@ class ConnectedDriveAccount:  # pylint: disable=too-many-instance-attributes
         self._refresh_token = None
         self._token_expiration = None
         self._log_responses = log_responses
+        self._retries_on_500_error = retries_on_500_error
+        self._retry_delay_on_500_error = retry_delay_on_500_error
         #: list of vehicles associated with this account.
         self._vehicles = []
         self._lock = Lock()
@@ -122,18 +132,30 @@ class ConnectedDriveAccount:  # pylint: disable=too-many-instance-attributes
         if headers is None:
             headers = self.request_header
 
-        if post:
-            response = requests.post(url, headers=headers, data=data, allow_redirects=allow_redirects, params=params)
-        else:
-            response = requests.get(url, headers=headers, data=data, allow_redirects=allow_redirects, params=params)
-
-        if response.status_code != expected_response:
-            error_description = ERROR_CODE_MAPPING.get(response.status_code, "UNKNOWN_ERROR")
-            msg = ("The BMW Connected Drive portal returned an error: {} (received status code {} and expected {})."
-                   .format(error_description, response.status_code, expected_response))
-            _LOGGER.debug(msg)
-            _LOGGER.debug(response.text)
-            raise IOError(msg)
+        for i in range(self._retries_on_500_error + 1):
+            if post:
+                response = requests.post(url, headers=headers, data=data, allow_redirects=allow_redirects,
+                                         params=params)
+            else:
+                response = requests.get(url, headers=headers, data=data, allow_redirects=allow_redirects,
+                                        params=params)
+
+            if response.status_code != expected_response:
+                if response.status_code == 500:
+                    _LOGGER.debug("Error 500 on attempt %d, waiting %d seconds before retry", i+1,
+                                  self._retry_delay_on_500_error)
+                    if self._retry_delay_on_500_error > 0:
+                        sleep(self._retry_delay_on_500_error)
+                    continue
+
+                error_description = ERROR_CODE_MAPPING.get(response.status_code, "UNKNOWN_ERROR")
+                msg = ("The BMW Connected Drive portal returned an error: {} (received status code {} and expected {})."
+                       .format(error_description, response.status_code, expected_response))
+                _LOGGER.debug(msg)
+                _LOGGER.debug(response.text)
+                raise IOError(msg)
+            break
+
         self._log_response_to_file(response, logfilename)
         return response
 
diff --git a/bimmer_connected/cli.py b/bimmer_connected/cli.py
index 22168c2..7246e4d 100644
--- a/bimmer_connected/cli.py
+++ b/bimmer_connected/cli.py
@@ -8,7 +8,7 @@ import os
 import time
 from bimmer_connected.account import ConnectedDriveAccount
 from bimmer_connected.country_selector import get_region_from_name, valid_regions
-from bimmer_connected.vehicle import VehicleViewDirection
+from bimmer_connected.vehicle import VehicleViewDirection, PointOfInterest
 
 FINGERPRINT_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)),
                                'vehicle_fingerprint')
@@ -41,6 +41,21 @@ def main() -> None:
     image_parser.add_argument('vin', help='vehicle identification number')
     image_parser.set_defaults(func=image)
 
+    sendpoi_parser = subparsers.add_parser('sendpoi', description='send a point of interest to the vehicle')
+    _add_default_arguments(sendpoi_parser)
+    sendpoi_parser.add_argument('vin', help='vehicle identification number')
+    sendpoi_parser.add_argument('latitude', help='latitude of the POI', type=float)
+    sendpoi_parser.add_argument('longitude', help='longitude of the POI', type=float)
+    sendpoi_parser.add_argument('--name', help='(optional, display only) Name of the POI', nargs='?', default=None)
+    sendpoi_parser.add_argument('--street', help='(optional, display only) Street & House No. of the POI',
+                                nargs='?', default=None)
+    sendpoi_parser.add_argument('--city', help='(optional, display only) City of the POI', nargs='?', default=None)
+    sendpoi_parser.add_argument('--postalcode', help='(optional, display only) Postal code of the POI',
+                                nargs='?', default=None)
+    sendpoi_parser.add_argument('--country', help='(optional, display only) Country of the POI',
+                                nargs='?', default=None)
+    sendpoi_parser.set_defaults(func=send_poi)
+
     args = parser.parse_args()
     args.func(args)
 
@@ -106,6 +121,15 @@ def image(args) -> None:
     print('vehicle image saved to image.png')
 
 
+def send_poi(args) -> None:
+    """Send Point Of Interest to car."""
+    account = ConnectedDriveAccount(args.username, args.password, get_region_from_name(args.region))
+    vehicle = account.get_vehicle(args.vin)
+    poi = PointOfInterest(args.latitude, args.longitude, name=args.name,
+                          street=args.street, city=args.city, postalCode=args.postalcode, country=args.country)
+    vehicle.send_poi(poi)
+
+
 def _add_default_arguments(parser: argparse.ArgumentParser):
     """Add the default arguments username, password, region to the parser."""
     parser.add_argument('username', help='Connected Drive user name')
diff --git a/bimmer_connected/const.py b/bimmer_connected/const.py
index 32b973d..13985f0 100644
--- a/bimmer_connected/const.py
+++ b/bimmer_connected/const.py
@@ -1,4 +1,4 @@
-"""urls for different services and error code mapping."""
+"""URLs for different services and error code mapping."""
 
 AUTH_URL = 'https://{server}/gcdm/oauth/token'
 BASE_URL = 'https://{server}/webapi/v1'
@@ -9,6 +9,7 @@ VEHICLE_STATUS_URL = VEHICLE_VIN_URL + '/status'
 REMOTE_SERVICE_STATUS_URL = VEHICLE_VIN_URL + '/serviceExecutionStatus?serviceType={service_type}'
 REMOTE_SERVICE_URL = VEHICLE_VIN_URL + "/executeService"
 VEHICLE_IMAGE_URL = VEHICLE_VIN_URL + "/image?width={width}&height={height}&view={view}"
+VEHICLE_POI_URL = VEHICLE_VIN_URL + '/sendpoi'
 
 # Possible error codes, other codes are mapped to UNKNOWN_ERROR
 ERROR_CODE_MAPPING = {
@@ -20,4 +21,4 @@ ERROR_CODE_MAPPING = {
     429: 'TOO_MANY_REQUESTS',
     500: 'SERVER_ERROR',
     503: 'SERVICE_MAINTENANCE',
-}
\ No newline at end of file
+}
diff --git a/bimmer_connected/exceptions.py b/bimmer_connected/exceptions.py
index f6ea977..f3151bb 100644
--- a/bimmer_connected/exceptions.py
+++ b/bimmer_connected/exceptions.py
@@ -1,4 +1,5 @@
-""" BMW ConnectedDrive exception class."""
+"""BMW ConnectedDrive exception class."""
+
 
 class BMWConnectedDriveException(Exception):
     """ BMW ConnectedDrive API Exception class."""
diff --git a/bimmer_connected/state.py b/bimmer_connected/state.py
index c230d65..c44757d 100644
--- a/bimmer_connected/state.py
+++ b/bimmer_connected/state.py
@@ -152,7 +152,7 @@ class VehicleState:  # pylint: disable=too-many-public-methods
     def gps_position(self) -> (float, float):
         """Get the last known position of the vehicle.
 
-        Returns a tuple of (latitue, longitude).
+        Returns a tuple of (latitude, longitude).
         This only provides data, if the vehicle tracking is enabled!
         """
         pos = self._attributes['position']
@@ -398,7 +398,6 @@ class Window(Lid):  # pylint: disable=too-few-public-methods
 
     A window can be a normal window of the car or the sun roof.
     """
-    pass
 
 
 class ConditionBasedServiceReport:  # pylint: disable=too-few-public-methods
diff --git a/bimmer_connected/vehicle.py b/bimmer_connected/vehicle.py
index e3cec37..57b030d 100644
--- a/bimmer_connected/vehicle.py
+++ b/bimmer_connected/vehicle.py
@@ -2,10 +2,12 @@
 from enum import Enum
 import logging
 from typing import List
+import json
+from urllib.parse import urlencode
 
 from bimmer_connected.state import VehicleState, WINDOWS, LIDS
 from bimmer_connected.remote_services import RemoteServices
-from bimmer_connected.const import VEHICLE_IMAGE_URL
+from bimmer_connected.const import VEHICLE_IMAGE_URL, VEHICLE_POI_URL
 
 _LOGGER = logging.getLogger(__name__)
 
@@ -52,6 +54,54 @@ class LscType(Enum):
     LSC_PHEV = 'LSC_PHEV'
 
 
+# pylint: disable=too-many-instance-attributes, too-few-public-methods
+class PointOfInterest:
+    """Point of interest to be sent to the vehicle.
+
+    The latitude/longitude of a POI are mandatory, all other attributes are optional. CamelCase attribute names are
+    used here so that we do not have to convert the names between the attributes and the keys as expected on the server.
+    """
+
+    # pylint: disable=too-many-arguments
+    def __init__(self, latitude: float, longitude: float, name: str = None,
+                 additionalInfo: str = None, street: str = None, city: str = None,
+                 postalCode: str = None, country: str = None, website: str = None,
+                 phoneNumbers: [str] = None):
+        """Constructor.
+
+        :arg latitude: latitude of the POI
+        :arg longitude: longitude of the POI
+        :arg name: name of the POI (Optional)
+        :arg additionalInfo: additional text shown below the address (Optional)
+        :arg street: street with house number of the POI (Optional)
+        :arg city: city of the POI (Optional)
+        :arg postalCode: zip code of the POI (Optional)
+        :arg country: country of the POI (Optional)
+        :arg website: website of the POI (Optional)
+        :arg phoneNumbers: List of phone numbers of the POI (Optional)
+        """
+        # pylint: disable=invalid-name
+        self.lat = latitude  # type: float
+        self.lon = longitude  # type: float
+        self.name = name  # type: str
+        self.additionalInfo = additionalInfo if additionalInfo is not None \
+            else 'Sent with ♥ by bimmer_connected'  # type: str
+        self.street = street  # type: str
+        self.city = city  # type: str
+        self.postalCode = postalCode  # type: str
+        self.country = country  # type: str
+        self.website = website  # type: str
+        self.phoneNumbers = phoneNumbers  # type: List[str]
+
+    @property
+    def as_server_request(self) -> str:
+        """Convert to a dictionary so that it can be sent to the server."""
+        result = {
+            'poi': {k: v for k, v in self.__dict__.items() if v is not None}
+        }
+        return urlencode({'data': json.dumps(result)})
+
+
 class ConnectedDriveVehicle:
     """Models state and remote services of one vehicle.
 
@@ -180,3 +230,14 @@ class ConnectedDriveVehicle:
             raise ValueError('Either latitude AND longitude are set or none of them. You cannot set only one of them!')
         self.observer_latitude = latitude
         self.observer_longitude = longitude
+
+    def send_poi(self, poi: PointOfInterest) -> None:
+        """Send a point of interest to the vehicle."""
+        url = VEHICLE_POI_URL.format(
+            vin=self.vin,
+            server=self._account.server_url
+        )
+        header = self._account.request_header
+        # the accept field of the header needs to be updated as we want a png not the usual JSON
+        header['Content-Type'] = 'application/x-www-form-urlencoded'
+        self._account.send_request(url, headers=header, data=poi.as_server_request, post=True, expected_response=204)
diff --git a/bimmer_connected/version.py b/bimmer_connected/version.py
index 83e147c..3c8c077 100644
--- a/bimmer_connected/version.py
+++ b/bimmer_connected/version.py
@@ -1,1 +1,3 @@
-__version__ = '0.6.0'
\ No newline at end of file
+"""Version information."""
+
+__version__ = '0.6.0'
diff --git a/requirements.txt b/requirements.txt
index 8163a79..8b95e5f 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,2 +1,2 @@
 requests
-typing>=3,<4
\ No newline at end of file
+typing>=3,<4;python_version<"3.5"
diff --git a/setup.py b/setup.py
index 2fb2cd3..6386923 100644
--- a/setup.py
+++ b/setup.py
@@ -19,7 +19,7 @@ setup(
     url="https://github.com/bimmerconnected/bimmer_connected",
     packages=['bimmer_connected'],
     install_requires=[
-        'requests', 'typing>=3,<4'
+        'requests', 'typing>=3,<4;python_version<"3.5"'
     ],
     keywords='BMW Connected Drive home automation',
     zip_safe=False,


Test patch:
diff --git a/test/__init__.py b/test/__init__.py
index cb2233c..898c75d 100644
--- a/test/__init__.py
+++ b/test/__init__.py
@@ -74,6 +74,30 @@ MISSING_ATTRIBUTES = [
     'sunroof',                    # not available in all vehicles
 ]
 
+POI_DATA = {
+    "lat": 37.4028943,
+    "lon": -121.9700289,
+    "name": "49ers",
+    "additionalInfo": "Hi Sam",
+    "street": "4949 Marie P DeBartolo Way",
+    "city": "Santa Clara",
+    "postalCode": "CA 95054",
+    "country": "United States",
+    "website": "https://www.49ers.com/",
+    "phoneNumbers": ["+1 408-562-4949"]
+}
+
+POI_REQUEST = {
+    "min": ("data=%7B%22poi%22%3A+%7B%22lat%22%3A+37.4028943%2C+%22lon%22%3A+-121.9700289"
+            "%2C+%22additionalInfo%22%3A+%22Sent+with+%5Cu2665+by+bimmer_connected%22%7D%7D"),
+    "all": ("data=%7B%22poi%22%3A+%7B%22lat%22%3A+37.4028943%2C+%22lon%22%3A+-121.9700289"
+            "%2C+%22name%22%3A+%2249ers%22%2C+%22additionalInfo%22%3A+%22Hi+Sam%22"
+            "%2C+%22street%22%3A+%224949+Marie+P+DeBartolo+Way%22%2C+%22city%22%3A+%22Santa+Clara%22"
+            "%2C+%22postalCode%22%3A+%22CA+95054%22%2C+%22country%22%3A+%22United+States%22"
+            "%2C+%22website%22%3A+%22https%3A%2F%2Fwww.49ers.com%2F%22"
+            "%2C+%22phoneNumbers%22%3A+%5B%22%2B1+408-562-4949%22%5D%7D%7D")
+}
+
 
 def load_response_json(filename: str) -> dict:
     """load a stored response from a file"""
diff --git a/test/test_vehicle.py b/test/test_vehicle.py
index 3e78dfd..138228f 100644
--- a/test/test_vehicle.py
+++ b/test/test_vehicle.py
@@ -3,9 +3,9 @@ import unittest
 from unittest import mock
 from test import load_response_json, BackendMock, TEST_USERNAME, TEST_PASSWORD, TEST_REGION, \
     G31_VIN, F48_VIN, I01_VIN, I01_NOREX_VIN, F15_VIN, F45_VIN, TEST_VEHICLE_DATA, \
-    ATTRIBUTE_MAPPING, MISSING_ATTRIBUTES, ADDITIONAL_ATTRIBUTES
+    ATTRIBUTE_MAPPING, MISSING_ATTRIBUTES, ADDITIONAL_ATTRIBUTES, POI_DATA, POI_REQUEST
 
-from bimmer_connected.vehicle import ConnectedDriveVehicle, DriveTrainType
+from bimmer_connected.vehicle import ConnectedDriveVehicle, DriveTrainType, PointOfInterest
 from bimmer_connected.account import ConnectedDriveAccount
 
 
@@ -72,3 +72,17 @@ class TestVehicle(unittest.TestCase):
                                           if a not in MISSING_ATTRIBUTES])
             expected_attributes = sorted([a for a in vehicle.available_attributes if a not in ADDITIONAL_ATTRIBUTES])
             self.assertListEqual(existing_attributes, expected_attributes)
+
+    def test_parsing_of_poi_min_attributes(self):
+        """Check that a PointOfInterest can be constructed using only latitude & longitude."""
+        poi = PointOfInterest(POI_DATA["lat"], POI_DATA["lon"])
+        self.assertEqual(poi.as_server_request, POI_REQUEST["min"])
+
+    def test_parsing_of_poi_all_attributes(self):
+        """Check that a PointOfInterest can be constructed using all attributes."""
+        poi = PointOfInterest(POI_DATA["lat"], POI_DATA["lon"], name=POI_DATA["name"],
+                              additionalInfo=POI_DATA["additionalInfo"], street=POI_DATA["street"],
+                              city=POI_DATA["city"], postalCode=POI_DATA["postalCode"],
+                              country=POI_DATA["country"], website=POI_DATA["website"],
+                              phoneNumbers=POI_DATA["phoneNumbers"])
+        self.assertEqual(poi.as_server_request, POI_REQUEST["all"])