msupply-foundation / msupply-cold-chain

Android application for viewing and monitoring temperatures of fridges
GNU General Public License v3.0
3 stars 3 forks source link

Retrieve temp logs without deleting logs from sensor #232

Open adamdewey opened 1 year ago

adamdewey commented 1 year ago

Is your feature request related to a problem? Please describe.

Due to bugs like #229 it would be a much safer bet to move to a model where we never tell the sensor to delete temperature logs and instead only retrieve the logs we require.

--> Losing temperature logs is a critical failure for the main use case for this product and we can't risk compromising the efficacy of client medical products. <--

Describe the solution you'd like

  1. App should request / retrieve only those temperature logs that it needs (the logs created since the last request).
  2. App should stop sending delete commands to the sensor.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

jmbrunskill commented 1 year ago

@adamdewey I assumed device storage capacity was the primary reason for deleting logs? If we never delete how long can logs be stored? I assume we'd need some kind of clean up process to makes sure we don't run out of space to store new logs...

adamdewey commented 1 year ago

@jmbrunskill - the sensors overwrite their last log on a rolling FIFO basis once they reach max capacity

andreievg commented 1 year ago

retrieve the logs we require App should request / retrieve only those temperature logs that it needs (the logs created since the last request).

This was not possible with BlueMaestro, maybe their API changed, maybe worth investigating. If we don't delete the logs, eventually logs will fill up on the device and we would always be downloading max logs. Due to that limitation keeping logs lean seemed beneficial for:

I can't say we've tested the battery life, but I do remember some tests with the later (it may not have been recorder or following strict scientific process, but I do remember some tested/real justification).

--> Losing temperature logs is a critical failure for the main use case for this product and we can't risk compromising the efficacy of client medical products. <--

Yes agree, our thinking was, we would only delete logs, if we've just integrated them (or is this also problematic ?). Unfortunately this is harder to validate given the technical path we've taken (using complex async logic with generators, where logic control flow is spread across different files and is hard to reason about).

Saying all of that I think there are two solutions: 1 - Don't delete logs, pull them all, make sure they don't overlap when integrating. 2 - Do delete logs but be very safe about it (logic in code to be easy to follow, and maybe extra check, download twice to validate temperature records match what's been saved). 3 - Research blueMaestor Api, maybe it's been improved to allow certain logs to be pulled (btw at the time this App was written, there was a parameter that could be set when pulling logs, to specify how many logs to pull, but it wasn't working)

anildahalsussol commented 1 year ago

@andreievg @adamdewey I did a pr : https://github.com/anildahalsussol/msupply-cold-chain/commit/68c29efd4558c62a6e1feed6f8d4ccd423c0a354

  1. Command update interval deletes the logs automatically for bluemaestor which was not for BT250. So I have downloaded the logs before updateloginterval command is fired.
  2. Second thing setting interval is allowed to do only once initially after that it will not update until the sensor is removed from the device.