OpenSeizureDetector / Garmin_SD

Garmin Watch Seizure Detector - A seizure detector data source based on Garmin IQ watches such as Vivoactive HR
http://openseizuredetector.org.uk
GNU General Public License v3.0
12 stars 8 forks source link

Retry mechanism doesn't work, and cannot work properly #54

Closed pmithrandir closed 7 months ago

pmithrandir commented 7 months ago

Today, the retry mechanism doesn't work. The only thing done in the on tick is to release the lock, allowing the next query to go.

Few thought : That release mechanism is redondant with the line

if (nSamp * SAMPLE_PERIOD == ANALYSIS_PERIOD) {

This line makes sure we cannot send 2 request before 5 samples are available. So the current system just block the next request if the timeout is not passed when first request failed. (not great)

2nd point. That line of code would be stronger if written >= in case something strange happens. (it should not, but if it was, it would be an infinite loop)

3rd point: Do we need a retry mechanism, and would it work. If data are sent every 5 sec, it might be enough to wait for the next batch to be sent, just raising an error: data not sent.

If we decide to retry, we should be aware that the data collected will not be accurate.

Normal situation: Data from sec 1, 2, 3, 4 and 5.

But if we would send data in the middle of the 5 sec loop mechanism, we would send 6, 7, 3, 4 and 5. I bet between 7 and 3 we could get false alarm.

My advice would be to

PS : I assigned the issue to you, but if you decide in one direction or the other, I can do the implementation.

jones139 commented 7 months ago

I did notice the other day that the 'retry' does not actually retry, it just displays a warning that the comms did not complete on time, so I agree it is not right. The check was really to prevent two web requests happening simultaneously - as you say it is a lock mechanism.

So for simplicity I agree that re-wording the error message and accept that we lose data if the comms have not completed is ok. I'd prefer it if you do the implementation because really I want to concentrate on getting the PineTime version released if possible (I'll do the code review for you). Thanks! Graham.

pmithrandir commented 7 months ago

Including in V2