Weather service improvements and improve memory usage #1860
Conversation
Enable C++ 17 so we can use new functionalities like "if constexpr".
Refactor WeatherService : - Add a sane max number of elements in the timeline - Create events in their own factory methods (CreateXXXEvent()) - Encapsulate QCBOR deserialization in template functions Get<>().
Disable the weather events that are not processed by InfiniTime right now using "if constexpr". We'll enable them when we need them.
Fix Temperature handling (uint16 -> int16) + use of optional<>.
Ensure that we keep only 1 version of each weather event in the timeline.
Ensure that PTS will update the weather info if temperature and cloud info were received. Assume 0 precipitation if no precipitation event were received.
|
Build size and comparison to main:
|
|
I really don't think the "one of the same type" approach is a correct way to invalidate events. I'd rather have bad companions wipe the timeline, than take that ability to display more than one number away from all of the companions. |
I don't think this is a good idea. I mean : how would companion apps know if they can push more data or not. How would they know how many data points InfiniTime can process, and how much will overflow the memory? I also don't think there are "bad" companion app : they simply use our API. The current API allows companion apps to let InfiniTime know that updated weather info are available. If they are able to provide updated info very often, all the better, the user will benefit from accurate weather info. On the other hand, I want InfiniTime to be as stable as possible, it must not crash. In this case, the crashes occur because the weather service stores all data coming from the companion app. Even if the older version of the data won't be used anymore. Even if the new data is exactly the same as the previous one. There must be a better way to handle this. In this PR, I tried to adopt a pragmatic approach based on our current use-cases : InfiniTime currently only processes temperature, cloud and precipitation. And the only thing it does with those data is to display a single data point. So there's no need to process and store other events, and multiple data points, since a single one can be displayed. Companion apps can send more events more often, that's fine, but the current version of InfiniTime will simply ignore those it cannot process. And in the future, when we have actual use-cases for those additional data, we'll be able to extend the current implementation to process them accordingly. And this will require no change on the companion app side! |
That's up to us to properly report. If the companion really operates on the principle of "we only know what's current, the past stuff doesn't matter and we don't know what's coming" then it would be better to start afresh.
If it's not supposed to be used any more it's supposed to be pushed with appropriate expiration. A companion that "lies" is, to us, a bad companion.
Fundamentally InfiniTime as a companion can't tell the difference between "the weather has remained unchanged" and "the data is the same". Information about validity/relevance has to come from the companion that actually has that data.
The plan was that PTS would display min-max, so already having just a single event would make that not possible. It's crippling the functionality before we actually get a chance to use it because a companion is sending junk and it can't be fixed.
It most likely would require changes, because they'd have no way to actually test out all the event types (and their validation). It's not a 100% guaranteed, but with stub functionality it's possible.
The other thing is exactly this. Next up would be calendar events. Sane limits would have to be figured out anyways, we can't just keep one event in the Calendar, that'd be Event :D |
FintasticMan
added
enhancement
As I said : we currently have no use for the past stuff, but this past stuff fills the memory and crashes the system. And since I cannot figure any use-case for those data, I thought it would be a good idea to drop them until we know how to use them. I'm just trying to provide a fix so that the feature we delivered actually works. We'll have the time to figure how to make future functionalities work... in the future. Could you explain why the past stuff is so important?
Please, let's try to remain courteous to companion apps and their developers. They do an amazing job integrating their apps with InfiniTime, and I don't think this is a great way to thank them for their work. As far as I know, all companion apps that integrate the weather functionality will cause a crash at one time or another. What should they have done to prevent that? I honestly do not understand your point here : they do not lie. They just say that at a specific moment in time, the temperature forecast for the next 6h is this value. Then, say 1 hours later, they send a new data saying that the new forecast for the next 6 hours is that other value. Is this a lie? No! It's an updated information. Should InfiniTime keep all the obsolete forecasts? I don't know, but we currently have no need for them right now.
The calendar use-case is an interesting example. I think your vision of the weather service and mine are quite different, and I have a hard time trying to understand how to fix the current issue with your explanations. You seem to have a good idea about how this should work, and I would be very interested in knowing more how we should improve the API, the weather service and the companion app to fix the issue and extend the functionalities. |
Expired events are not important. But that's the thing, a companion pushing 6h every 1h will cause a bunch of useless overlap. The events are technically still valid. InfiniTime doesn't know which one to purge practically. "Let's take the oldest and always wipe that" will result in "we can't even display current min-max". Which would be a pity.
It's not a guarantee, they might though. If they keep pushing at a higher rate than gets invalidated.
But they could know their refresh interval and just say that this forecast is valid until the next refresh (+ grace period). It Would prevent a 5x-10x resource consumption. I think from InfiniTime's perspective it is a lie if an event always becomes invalid 5h early.
That's fairly easy, there's likely some kind of unique identifier per calendar event. It's possible to do a targeted replacement. We don't have that for forecasts. Especially if we already can't get accurate expiration it'd be a bit much to ask for.
Which applies here too, instead of just 1.
Fundamentally InfiniTime shouldn't take on this task of forecasting how long a forecast will last. It's one of the basic requirements we have to have from the companions, to get valid data. Right now it's expiricy, next it's stable unique identifiers. I can probably submit a hotfix to Gadgetbridge to restrict all events to 1h until WeatherSpec is amended to contain the information needed. Secondly we shouldn't limit it to just one event of the same type. Rather set a limit to the total, to an amount we can actually (likely) store. Thirdly, we can finalize the timeline control API, let them purge the timeline and start afresh if it's too much for a companion to track. That way we don't have to significantly restrict most use-cases. |
Does this mean that they lock themselves out of pushing new values for the time they promised their data to be valid (if they are being nice)? One middle ground between you two seems to be to tag the data with a category id and allow at most one datum per category. This ensures flushing updated values, but would give you multiple temperatures for different purposes for example. |
No. But they might if a timeline total size is enforced. (A fresher event with the same expiration should override when selected. We would just like it not to happen 5x-10x with each event.) |
|
I tested this branch and it seems, that the weather is not updated if newer or data from other location data is available and send to the watch. Just the very first time (if you restart the watch) or if the data expires, it works. |
|
Another thing, maybe out of scope: I am using Gadgetbridge and BreezyWeather as weather app. The cached weather data in Gadgetbridge looks like following:
You can see, it is raining. My question: why is not the "Condition Code" used to decide which weather icon is displayed? |
Sorry, that's not the topic of this PR. Rather a separate issue. Without GB log it's also not possible to say if precipitation got pushed to the timeline or not. |
|
Ok, i will create a new issue for that |
|
I do not use the weather feature, and I have no need for it, so I might do some incorrect assumptions when I try to understand the use-cases of this functionality. In my mind, the most basic use-case (the one that is currently integrated in InfiniTime), consists in displaying the current weather (temperature, cloud, precipitation). The "value" displayed in InfiniTime should basically be the same than the one displayed in the companion app/weather app. If the companion app refreshes the data, I expect to see this updated data on my PineTime. Not the older value, not a processed value from the previous ones and the new one, I expect my PineTime to reflect exactly the data displayed in the companion app. If I connect my PineTime to another companion app that uses another weather data source, the forecast might be different than the first one, but I still expect my PineTime to show the same info than the companion app it's connected to. From what I can tell, this simple use-case does not work : the data displayed is not always the same than the one from the companion app and InfiniTime will eventually crash because of memory overflow. I've already explained why in this post and in #1822. Before even thinking about more advanced use-cases, I would like to ensure that our design can at least support this basic use-case. I don't like to say this, but I would like to make it clear that the InfiniTime 1.14 will integrate a weather service that is correct (regarding memory management and data displayed) by design, or no weather service at all. I think this PR could be a good temporary workaround until we improve the implementation of the weather service. The only thing that's missing is to define how to select which single data point will be stored and displayed in the watchface. I'm however willing to improve this PR, implement another solution or even rewrite the weather service from scratch, but I'll need much more precise use-cases and information on how we want the data to be processed by InfiniTime. |
This would require implementing the control endpoint (that's currently just a stub) to provide a method for wiping cached data. Always wiping the previous data or the previous element of the same type will not work. As described previously.
Absolutely can support, if not for buggy companions.
It would cripple it when there would be better approaches. |
|
I'm in favour of the watch managing the data stored on the timeline and pruning it in a (hopefully) intelligent way. Making changes to companion apps is vastly more complex than to InfiniTime, and unless we have a clear spec of exactly what we want to recieve, we can't reasonably expect companion apps to deliver only what we want. In terms of min/max data, I think this is non-viable purely from the timeline for a few reasons, given expired events are purged, the best we can offer is min/max temp for some period of time, which is dependant on the data provided by the companion app, it could be 1h or 6 or 24, and if it's not clear what period the min/max values cover, it's not useful data. An option might be to store the highest and lowest values seen on the timeline over a 24h period, but if we can't store 24h of data, we can't count on the timeline to provide realistic min/max values. Static expiry times are already less than ideal, if we could get valid expiry times via GB we would be in a much better situation. IMO, GB also needs not to push the 24h temperature data point, it serves no purpose and only confuses things. As it stands we are only aiming to display the current temperature, cloud and precipitation data. The watch dropping all other data types seems like an obvious step, even if it is less than ideal from the viewpoint of being able to extend the weather system in the future, but if we're doing the dropping, we can also change that easily later. I vote to make it work with what we want to do now, without making companion apps modify their behaviour which may need reverting in the future. |
Fixing the expiration time needs WeatherSpec changes, but we didn't get those right now. Which is very unfortunate. This is not a InfiniTime's problem to solve.
I disagree, having written the current support in GB. The control endpoint was also specifically defined to accommodate functionality mentioned in my previous comment, it has not just been implemented because we couldn't predict what features would be useful. Now we do.
It's only a guess that it's 24h, to be fair. But incorrect data being pushed is indeed confusing. I will prepare a PR to slash the expiration times and modify that one in GB so that they won't cause any issues.
I did intend and hopefully document it well enough that events should expire when they're no longer valid. Knowing that the refresh period is smaller than the set expiration time means that a companion is essentially sending incorrect data. We shouldn't expect pre-filtering of event types, but we should be able to expect simply correct data.
I'm fine with dropping, just to be clear, it just has to be after validation and not return an error. |
|
I'm closing this PR since I don't think we'll be able to fix all the issues that were mentioned. I'm currently in the process of re-designing the service and the weather functionality and I hope to come up with a solution soon. |
This branch implements the suggestion I did in this comment in #1822:
Additionally, it also improves PTS : it now refreshes the weather info even if it didn't receive any precipitation event (assume no precipitation in that case).
I'm not satisfied with the policy that decides which "version" of the event will be stored in the timeline. The policy I implemented here
I'm open to suggestions for a better policy :)