Tankerkoenig shows state as 'unknown' for closed fuel stations

Vedeneb commented 4 years ago

The problem

tankerkoenig integration displays the state as "unknown" if the fuel station status is closed.

I don't see a reason for that, I just checked the API and it displays all information, also if the station is closed:

{ id: "xxxx", name: "bft- Tankstelle xxxx", brand: "BFT", street: "xxxx", place: "xxxx", lat: xxxx, lng: xxxx, dist: 6.3, price: 1.129, isOpen: false, houseNumber: "80", postCode: xxxx },

I have set the logger level for tankerkoenig to debug and found that when updating for closed station nothing but this is logged: 'xxxx-station-id': {'status': 'closed'}

meanwhile for open stations everything is logged as supposed: 'xxxx-station-id': {'status': 'open', 'e5': 1.219, 'e10': 1.189, 'diesel': 1.129}

Environment

Home Assistant release with the issue: 0.107.5
Last working Home Assistant release (if known): None, but problem also existed with 0.107.1
Operating environment (Hass.io/Docker/Windows/etc.): Manual installation on a Intel NUC
Integration causing this issue: tankerkoenig
Link to integration documentation on our website: https://www.home-assistant.io/integrations/tankerkoenig/

Problem-relevant `configuration.yaml`

tankerkoenig:
  api_key: my-api-key
  radius: 5
  scan_interval: 600
  fuel_types:
    - "diesel"

Traceback/Error logs

The log shows no errors, only debug messages as shown above.

Additional information

probot-home-assistant[bot] commented 4 years ago

Hey there @guillempages, mind taking a look at this issue as its been labeled with a integration (tankerkoenig) you are listed as a codeowner for? Thanks!

guillempages commented 4 years ago

There are three methods in the Tankerkoenig API; one to get the stations inside the radius, one to get data about individual stations and one to batch-get data about multiple stations at once.

The first two methods deliver information also for closed stations, but the batch methods did not deliver price information for closed stations. In order to not saturate the API with calls, the first methods are only used in the initialization; the regular updates use the batch get methods, and thus, no price information is received for closed stations.

What's the point in displaying the price, if the station is closed? When the station items, the price is normally much higher than the last price anyway.

Vedeneb commented 4 years ago

Thanks for the answer. I don't see why the API would have to be requested more often using the radius-method. Sure, the response would contain a bit more stuff but I'm pretty sure tankerkoenig won't have a problem with it? From what I read they are only concerned about the frequency the API is requested.

Why I wan't prices for closed stations? Well, first of all because I want complete statistics, I want a chart showing me how the price developed during the days. Also many fuel stations have only opened very few hours a day. Some are even completly closed on some days (most on sunday) but I of course still want to be able to see the prices.

Even if you decide to stay at the batch-API-method I think the state of the sensor should at least stay at the price from the last request when the station had opened. Having a sensor's state saying 'unknown' it in most other cases means something went wrong and I think that's the way the 'unknown' state is meant to be used. Not sure about that point though.

Still, thanks for bringing this integration to Home Assistant!

guillempages commented 4 years ago

The integration also offers the possibility to manually add stations to track, via "additional_stations". Those stations won't be returned via the radius-method, and asking them individually would increase the frequency the API is requested. And using the batch-method for the additional stations and the radius-method for the automatic stations would somehow create a two-class system, where some stations have prices when closed and others do not, which would be much worse.

I can understand, that you want complete statistics, but while the station is closed, the statistic is meaningless. The price could be set to e.g. 0.5€ and it would not be less true. You can still see the chart, and during the time the station is closed, no price will be displayed; but the rest of the day is displaying the price normally. But of course, this is a matter of opinion/taste.

As far as I understood the "unknown" state, it doesn't necessarily mean "error"; it just means "no data available", which is true in this particular case. If there are more complaints about this, I might rethink it, but for the moment I'd prefer to keep the implementation as-is.

Thanks for using the integration; I was myself not sure how many people would actually use this (I implemented it without a use-case in mind; just to learn how to create an integration), but from the comments I can see this is something some people did want. :-)

balloob commented 4 years ago

I agree that unknown is right here, because we don't know what the price is.

Vedeneb commented 4 years ago

Thanks for your answers. I understand your points. Have a great day and stay healthy!

home-assistant / core