search

qistoph / MMM-MyCommute

This a module for the MagicMirror. It shows your commute time using Google's Traffic API
38 stars 14 forks source link

Bug: startTime delayed (max pollFrequency) #2

Open qistoph opened 5 years ago

qistoph commented 5 years ago

An update cycle is run periodically (see pollFrequency). The module is only shown if during this update cycle the current time is between startTime and endTime.

It would be better if the module would be shown as soon as the current time reaches startTime.

radokristof commented 4 years ago

I already had some times that it won't even show the module even if it is still in the window and several pollFrequency updates made.

qistoph commented 4 years ago

Any idea why?

radokristof commented 4 years ago

Not really, I haven't looked into it yet. I just discovered that the module should be visible and it was still hidden, even 1 hour after it should be on. A simple MagicMirror restart solved it, so I don't really get why it was still hidden. Looking at the code, it should check this every pollFrequency... I did some enhancements to the code, I will create a PR.

radokristof commented 4 years ago

I have probably found the issue: https://github.com/qistoph/MMM-MyCommute/blob/609007df6a02d853b5dbf2f1b566f70a9debcf81/MMM-MyCommute.js#L534

Here change hidden to isHidden

qistoph commented 4 years ago

WOW! Thank you so much! I don't have time to test it at the moment, but it definitely looks really suspicious.

radokristof commented 4 years ago

I'm still testing it, but it seems that this solved it for me. Regarding the original issue, I thought about this, but I'm not sure how it can be achieved with the current code design. What I thought of is using cron jobs (for each destination) and if it is started by cron, simply start the timer at that time. However this means that every destination needs to be seperated and ran in its own thread with it's own interval (which is not so bad, you could specify pollFrequency for each destination), but it needs a major overhaul. Also ending is not so obvious with this. Maybe another cron which cancels the timer when reached...

qistoph commented 4 years ago

I was thinking something along:

At any time it's possible to calculate how long until the module needs to be shown. That could be used as delay before showing. If we set that as the delay until next update (instead of pollFreq.), the module would update and show at that time.

radokristof commented 4 years ago

Oh, that seems easier... However you can do this only for the global config startTime and endTime (because only one "timer" running now). Or you calculate for each destination and use the closest one (and after you just use the pollFrequency). If isHidden is true again, then you calculate the closest one again... However I don't know how you can manage calendarOptions in this way.

radokristof commented 4 years ago

@qistoph Unfortunately, I'm my module still won't show up sometimes (even after changing that isHidden parameter - also there is a default hidden parameter also which is changes to true when you hide the module, so this shouldn't be a problem). What I can't figure out right now is when you hide the module and the suspend method is called, you clear the interval there, which means there is no timeframe check and polling. However what will trigger the resume function (in order to start polling again)?

qistoph commented 4 years ago

I have probably found the issue:

https://github.com/qistoph/MMM-MyCommute/blob/609007df6a02d853b5dbf2f1b566f70a9debcf81/MMM-MyCommute.js#L534

Here change hidden to isHidden

That seems to fix it indeed! Thank you so much! Patched and pushed.

radokristof commented 4 years ago

For me unfortunately not. It was still times when it didn't wanted to show up. I have simplified the code as much as I can, I'm testing it, but it seems to be working. The problem seems correct, that removing the interval in suspend, the polling will stop completely, that's why it will not show up after all destinations are hidden.

qistoph commented 4 years ago

Hmmm, that might have been intentional. Since the module is using credits I wanted people to have control over the amount of credits the module is using. By limiting the visible time, the idea was, to also limit the amount of credits used. Maybe a complete redesign would be a better start...

radokristof commented 4 years ago

Yes that's right. However my approach will not change this behavior, the amount of credit used will be the same. Assume you have a 10 min pollFrequency set, you poll every 10 min. If the getData function starts, it will check if any destination is in the timeframe specified, and if not there will be no data sent to Google. However if you remove this 10 min pollFrequency completely, you can't check every 10 min if is there any destination in the timeframe.

radokristof commented 4 years ago

I have eliminated all of the module not appering bug. It works for a week now without a problem. I can make a PR is you like that, however my changes brings some breaking API changes (like the global hide/show is not there - this is just a duplicate so I don't think it has any benefit, and other things). Also if the alternative route name is too long and overlaps, this has also been solved by truncating the text.

qistoph commented 4 years ago

Yes that's right. However my approach will not change this behavior, the amount of credit used will be the same. Assume you have a 10 min pollFrequency set, you poll every 10 min. If the getData function starts, it will check if any destination is in the timeframe specified, and if not there will be no data sent to Google. However if you remove this 10 min pollFrequency completely, you can't check every 10 min if is there any destination in the timeframe.

That sounds nice indeed.

I have eliminated all of the module not appering bug. It works for a week now without a problem. I can make a PR is you like that, however my changes brings some breaking API changes (like the global hide/show is not there - this is just a duplicate so I don't think it has any benefit, and other things). Also if the alternative route name is too long and overlaps, this has also been solved by truncating the text.

A PR sounds nice. I would however like to keep supporting the global show/hide, because that is probably in use in other people's setups. I'm also using it to show/hide modules using the Remote Control module. It's also used in modules that support user profiles (facial recognition to show a person's own modules).

radokristof commented 4 years ago

Ok I will create the PR soon.

I would however like to keep supporting the global show/hide, because that is probably in use in other people's setups. I'm also using it to show/hide modules using the Remote Control module. It's also used in modules that support user profiles (facial recognition to show a person's own modules).

Hmm, I may not be right, but I think this change still allows these. When you use Remote Control you can still show hide the module, because it will call the hide method (that is still there). I don't really know how the profiles work, but I assume that you have to specify the modules or destinations for each user. That will still work. However you are right, this also will bring another API breaking change... At least for those who just use the global show hide and nothing for destinations (because than it will show the destinations always). Maybe you could try my fork how it works in your scenario like this.