Fix Here Travel Time unable to find entity on startup

[eifinger]

Description:

@MartinHjelmare we discussed to remove pattern matching for origin/destination here.

I did however find out that this leads to uncaught errors when origin or destination is a template sensor and the state at the time of the update is not a valid coordinate:

[homeassistant.helpers.entity] Update for sensor.reistijd fails
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 261, in async_update_ha_state
    await self.async_device_update()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 439, in async_device_update
    await self.async_update()
  File "/config/custom_components/here_travel_time/sensor.py", line 277, in async_update
    await self._hass.async_add_executor_job(self._here_data.update)
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/config/custom_components/here_travel_time/sensor.py", line 361, in update
    origin, destination, [self.route_mode, self.travel_mode, traffic_mode]
  File "/usr/local/lib/python3.7/site-packages/herepy/routing_api.py", line 78, in car_route
    return self._route(waypoint_a, waypoint_b, modes)
  File "/usr/local/lib/python3.7/site-packages/herepy/routing_api.py", line 54, in _route
    'waypoint1': str.format('{0},{1}', waypoint_b[0], waypoint_b[1]),
IndexError: list index out of range

Related issue (if applicable): fixes #27465

Pull request with documentation for home-assistant.io (if applicable): N/A

Example entry for configuration.yaml (if applicable):

N/A

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.
• I have followed the development checklist

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

• N/A

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

• The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

If the code does not interact with devices:

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

[MartinHjelmare]

Can't we just catch the error when a template sensor doesn't render a valid value, and not continue the update?

[eifinger]

This is exactly what I implemented here. If the value is an entity_id it will recursively try to resolve it. If the value is in the form lat,lon it will directly use it. All other values will throw an error and end the update.

[eifinger]

I just implemented a feature to catch circular references. Will push this later today.

[MartinHjelmare]

I mean that instead of changing the behavior, we just catch errors and log them. The user can define a template sensor that returns coordinates as needed in the state attributes.

Why do we need to nest down when looking up entity_id?

[eifinger]

I prefer to check the argument over catching an IndexError which might occur anywhere in the code.
Moreover this adds an additional, user requested feature to use entities like input_select directly without the need to create an additional template sensor. We already do this for all the other domains in the TRACKABLE_DOMAINS list.

I do understand that request as it simplifies things a lot and template sensors have the tendency to be unknown at startup which leads to the case that the depending here_travel_time sensor will only display a correct state after his second update.

[MartinHjelmare]

We can postpone the first update until home assistant start event.

We should avoid overloading options or code as much as possible.

[eifinger]

Postponing would definitely help with the template sensors.
I really do want to implement the entity_id resolution because the hassle with extra template sensors annoys me too. Do you see another way that does not use method overloading?

[MartinHjelmare]

I think the user should use a template sensor for this case. We keep the interface clear and simple.

[eifinger]

I am still trying to understand why we don't want users to have this feature but force them to create an additional entity and learn templating.

When you say interface, are you referring to the function interface? What do you think should be altered? The recursive nature as such or the following part?

recursion_history=None

As for the regex matching for correct lat/long set: If we implement it this way instead of catching the exception we don't have to rely on the implementation details of the external lib. If the version gets upgraded to use new features and this bug was caught in the lib itself we would have to test for another exception. At least that was my line of reasoning 😄

[MartinHjelmare]

When I said interface I meant the integration config option interface that the user uses.

[MartinHjelmare]

I would prefer if we use our validators. They are more clear.

https://github.com/home-assistant/home-assistant/blob/a3c98440e0e208a162f90ad67a5f843e5011f153/homeassistant/helpers/config_validation.py#L70

[MartinHjelmare]

Remove the nesting check. We should keep the options interface simple.

[eifinger]

Thank you for the voluptuous hint! Haven't thought of that.

The options don't change for the user.
Taking origin as an example:

Before this PR he was allowed to use either origin_coordinates or origin_entity_id.
For origin_entity_id the domains device_tracker, sensor, zone, person were allowed.

With this PR the user can also use the domains input_select and input_text.

So the user does not get a new option for configuration, this PR is rather removing the constraint on the allowed domains for origin_entity_id

[MartinHjelmare]

The behavior of the option changes. The user needs to take this into consideration. The user needs to know what we will do with the entity_id to set up the input select.

I don't think this additional complexity is necessary. Reading the docs about the existing options I think we're already too complicated.

[eifinger]

I agree that this behavior is complex and a new user will not understand it directly. He will just follow the simple example in the documentation using fixed coordinates.
However the user I linked in the beginning and also myself a few months ago stumbled across the current behavior of all travel_time_components.
The line of thought is that one should be able to use an input_select / sensor state directly. Instead of creating a template sensor with a value_template to check if the state is the name of e.g. a zone and if yes then access the latitude and longitude attributes of that zone entity.

In my opinion that complexity is harder to understand than the handling this PR tries to implement.

That being said, this is just my opinion and I thought I was on the right track because I wasn't the only one requesting this feature.

If you think homeassistant will be better of without this feature please feel free to close the PR. I will implement the state parsing with voluptuous instead of regex in a new PR.

[MartinHjelmare]

I'm happy to hear more opinions from other members.

[pathia]

I was the user requesting this change in the first place. The way @eifinger implemented was the way I expected it to work in the first place..
To be honest I found it rather odd that it wasn't accepting my dynamic destination. This way I am able to have my travel_time continuously updated based on decisions through automations and/or Jinja2 templates throughout my day/week.

[Pirol62]

From the use case point of view I can only agree to this solution. I'm travelling a lot and my destination changes nearly daily during working days. An input_text or automation which reads my calendar would give the needed flexibility to decide easy, where to go now and its much faster then opening google maps or whatelse

[eifinger]

Second Opinion from balloob in discord:

we should really create a component that does the resolving and has platforms from other integrations to do the calculation
[...]
I think that we should make a generic travel time integration
And we have a Waze platform, a here platform
Etc

Based on this I removed the nested entity resolving and will readd it as part of a travel_time integration.

This PR now "only" fixes #27465.

[MartinHjelmare]

Looks good!