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

Fix issue with 'async_request_refresh' #40

Merged
merged 3 commits into from
Jun 20, 2021
Merged

Conversation

tdragon
Copy link
Contributor

@tdragon tdragon commented Jun 17, 2021

No description provided.

This call generates a HTTP query on every access to sensor or forecasts.
Basically it forces HA to refresh forecast many times a second insted of
once in 30 minutes.

+ add unique id to all sensors
+ add a separate weather service for daily mode if enabled
Copy link
Contributor

@TheStork239 TheStork239 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think FMI have recently changed the OAAS sea level query so that it now returns two types of sea level measurements, with the parameters in the query being called ‘SeaLevel’ (as it was before) but also now a new ‘SeaLevelN2000’ measurement. So I think that a “starts with SeaLevel” type of parsing line might cause unintended results as it would presumably pick up both of the measurement types from the data?

@TheStork239
Copy link
Contributor

Hi again @tdragon,
One way would be to just add an 'else if' to skip over the entries that are in fact for the new 'SeaLevelN2000' parameter, something along the lines of the below:

for n in range(len(root_mareo)):
    try:
        if root_mareo[n][0][2].text == 'SeaLevel':
            tuple_to_add = (root_mareo[n][0][1].text, root_mareo[n][0][3].text)
            sealevel_tuple_list.append(tuple_to_add)
        elif root_mareo[n][0][2].text == 'SeaLevelN2000':
            pass
        else:
            _LOGGER.debug("Sealevel forecast record mismatch - aborting query!")
            break
    except:
        _LOGGER.debug(f"Sealevel forecast records not in expected format for index - {n} of locstring - {loc_string}")

@tdragon
Copy link
Contributor Author

tdragon commented Jun 19, 2021

Good point. I am not really interested in the SeaLevel, but seen an error in the log and quickly fixed it.
I've updated my fix, I think it is safe to ignore everything unexpected. What do you think?

@TheStork239
Copy link
Contributor

Thanks. I think a potential issue with your amended fix is that it will flood the log with “unsupported record” lines. The FMI sea level data fetch brings in half hourly forecast data for two days ahead, and for each time point it now has both a ‘SeaLevel’ and ‘SeaLevelN2000’ entry. So every time the sensor updates, you would see nearly a hundred log lines on unsupported records.

I think the parsing over the forecast would best work if it followed the below logic:

  1. If it finds a ‘SeaLevel’ entry, it adds the tuple. (This works in current code already).
  2. If it finds a ‘SeaLevelN2000’ entry, it skips it without creating a tuple. These ‘SeaLevelN2000’ entries are now part of what the FMI mareograph data fetch query returns, but it is expected and probably do not need to be reflected in the logs.
  3. If it finds some other entry than either ‘SeaLevel’ or ‘SeaLevelN2000’ then something has gone wrong with the data fetch and that should be highlighted in the logs.

That way it would only create a log entry if something unexpected came up when parsing the data.

@tdragon
Copy link
Contributor Author

tdragon commented Jun 20, 2021

Ok, I added a skipping of the ‘SeaLevelN2000' without logging.

Copy link
Owner

@anand-p-r anand-p-r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for changing this!

@anand-p-r anand-p-r merged commit 805a14c into anand-p-r:develop Jun 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants