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

omSupply Cold Chain Temperature Sensors #1955

Closed
19 of 20 tasks
andreievg opened this issue Jun 29, 2023 · 11 comments
Closed
19 of 20 tasks

omSupply Cold Chain Temperature Sensors #1955

andreievg opened this issue Jun 29, 2023 · 11 comments
Assignees
Labels
Epic feature: coldchain needs architecture/solution Needs wider dev input on general solution

Comments

@andreievg
Copy link
Collaborator

andreievg commented Jun 29, 2023

Background

We have a number of actors in mSupply ecosystem that deal with temperature records, their core functionality:

  • Capture temperature data from sensors
  • Record and sync this data to central server
  • Calculate and acknowledge temperature breaches
  • Display temperature logs and temperature breaches in comprehensible format
  • Notification of abnormal temperature exposures and breaches
  • Link LMIS data with temperature log data

Hardware Configurations

All of the above can make sensor data available on central server (via our sync system), which in turn allows for dashboard via Grafana and notifications via Grafana telegram integration.

Current scope for omSupply (requirements)

For MVP Temperature Sensors in omSupply:

  • Read Berlinger fridge tag data by omSupply
  • Integrate mSupply Cold Chain app data to omSupply
  • This data to sync to central server
  • Simple display of temperature data (tabular)

Tasks

Should have (Phase 2)

Nice to have

Technical details

Please see existing schema for mSupply Mobile and mSupply Cold Chain app below:

Temperature Breach Configs don't sync from mSupply Cold Chain app but do sync from mSupply Mobile, but this info is available in temperature breach.

Otherwise schemas are exactly the same 🎉

And improvement would be to save sensor battery level (history).

Also @adrianwb suggested that it might be better to calculate the link between temperature log and temperature breach on the go (rather then maintaining the link).

mSupply Cold Chain

temperature_log
https://github.com/openmsupply/msupply/blob/6387c070dc5f2ba8edc8e578aebed2577ae4f17a/Project/Sources/Methods/coldchain_temperatureLogV1.4dm#L59-L77

temperature
temperaute_breach_ID
date
time
log_interval
sensor_ID
location_ID
store_ID

temperature_breach
https://github.com/openmsupply/msupply/blob/6387c070dc5f2ba8edc8e578aebed2577ae4f17a/Project/Sources/Methods/coldchain_temperatureBreachV1.4dm#L52C5-L77

start_date
start_time
end_date
end_time (what if ongoing and not liking temperature logs ? )
acknowledged
threshold_maximum_temperature
threshold_minimum_temperature
threshold_duration
type: HOT/COLD_CONSECUTIVE, cumulative breaches is a 'static' thing in cold chian app, see breach explanation
sensor_ID
location_ID
store_ID

sensor
https://github.com/openmsupply/msupply/blob/6387c070dc5f2ba8edc8e578aebed2577ae4f17a/Project/Sources/Methods/coldchain_sensorV1.4dm#L53-L68

macAddress
name
batteryLevel
storeID
locationID
is_active
log_delay_time
log_delay_date
logInterval
programmed_time
programmed_date

Mobile

https://github.com/openmsupply/mobile/blob/91b4e0778c8a2916803ad621c4b52ecd81301f4f/src/sync/syncTranslators.js#L87-L89

temperature_log
https://github.com/openmsupply/mobile/blob/91b4e0778c8a2916803ad621c4b52ecd81301f4f/src/sync/outgoingSyncUtils.js#L314-L321

temperature
temperaute_breach_ID
date
time
log_interval
sensor_ID
location_ID
store_ID

temperature_breach
https://vscode.dev/github/openmsupply/mobile/blob/develop/src/sync/outgoingSyncUtils.js#L327-L338

start_date
start_time
end_date
end_time (what if ongoing and not liking temperature logs ? )
acknowledged
threshold_maximum_temperature
threshold_minimum_temperature
threshold_duration
type: Same as temperature_breach_config, but not sure how it's calculated: https://github.com/openmsupply/mobile/blob/91b4e0778c8a2916803ad621c4b52ecd81301f4f/src/bluetooth/BreachManager.js#L27
sensor_ID
location_ID
store_ID
temperature_breach_config_ID - Desktop doesn't get this from cold chain app

temperature_breach_config
https://github.com/openmsupply/mobile/blob/91b4e0778c8a2916803ad621c4b52ecd81301f4f/src/sync/outgoingSyncUtils.js#L344-L351

minimum_temperature
maximum_temperature
duration
description
colour
type: HOT/COLD_CUMULATIVE/ or HOT/COLD_CONSECUTIVE
location_ID
store_ID

sensor
https://github.com/openmsupply/mobile/blob/91b4e0778c8a2916803ad621c4b52ecd81301f4f/src/sync/outgoingSyncUtils.js#L379-L389

macAddress
name
batteryLevel
storeID
locationID
is_active
log_delay_time
log_delay_date
logInterval
programmed_time
programmed_date

Breaches

Although breach calculation is out of scope of this issue, adding some extra info about breaches (both Berlinger fridge tag and mSupply cold chain app calculate their own breaches)

See this doc for details.

It looks like in mSupply Cold Chain cumulative breaches are not recorded as breach (they are displayed based on the period that use is looking at), as per this doc

In the time that I've spend looking at mobile code I didn't see if and how cumulative breaches are calculated.

Code removal (to be added back in eventually)

15cc88d
286d82d
#2375

@DhanyaHerath DhanyaHerath added front-end Front-end related back-end Back-end related needs architecture/solution Needs wider dev input on general solution and removed needs triage front-end Front-end related back-end Back-end related labels Jul 5, 2023
@adrianwb adrianwb self-assigned this Jul 25, 2023
@adrianwb
Copy link
Contributor

FYI - I've created a new branch for this and started on the basic stuff to add temperature sensor support into omSupply, based largely on existing similar code for locations (nearest existing match)

@andreievg
Copy link
Collaborator Author

@adrianwb, thanks for starting on this !

It would be great if you can think of a way to PR this in bits. Our usual process, is an epic is created (this one). Then tasks are made into issues (and linked in the epic), and PR are made from those issues.

I've tried to break up the task list to help with the breakdown of tasks, from front-end/back-end experience, this implementation should roughly fit the tasks, but sometime it strays away (tasks may expand/contract the context).

Would be great if you can make PRs as those areas are done, this would help us with getting you feedback as soon as possible and reduce conflicts form develop branch (you would need a feature branch most likely). Also we have a general ethos of reducing PR.

Also we have general contributor guidelines in the wiki:
https://github.com/openmsupply/open-msupply/wiki/Code-Review-Comments---General
https://github.com/openmsupply/open-msupply/wiki/Code-Review-Comments---Rust
https://github.com/openmsupply/open-msupply/wiki/Code-Review-Comments---Typescript

Lastly, I've made a comment here:

  • It's probably best for now to just manually select the file (would need upload endpoint I think), for now I guess it could just be a dummy end point with hard coded data on server.

Please let me know if you want to chat about this in person, might be easier to align our understanding of the implementation and share some omSupply dev and dev tool tips

@adrianwb
Copy link
Contributor

adrianwb commented Aug 8, 2023

I've made the crate public and confirmed that I can pull it into omsupply server and build it successfully.

Rather than split things up vertically by areas of functionality, I'd thought it would be easier for me as a newbie to work horizontally (start with sensor and then add in temperature logs and breaches, as I don't yet know enough about the code structure to split it up by area) => so far, I've added in the sensor schema and handling pretty much everywhere within server (at least all the places that I can find, and there are a lot!) in the https://github.com/openmsupply/open-msupply/tree/1955-omSupply-Cold-Chain-Temperature-Sensors branch, so easiest to use that as the base feature branch? It all compiles up now (server + tests) although I haven't yet done any real testing with an actual 4D server file - that's next on my list, but all being well, sensors should sync with 4D and be available in graphql queries.

Once that's done, I'll create sub-branches to add in the temperature logs, breaches and breach configs, and then link in my crate to pull in live data from USB and create the service API to translate it into the DB schema.

It's been quite the learning experience, but slowly starting to get easier :-) The client/UI side of it will be another set of things to figure out - hopefully there are enough examples to work from, but the problem I often find with docs and examples is that there is a lot of technical detail, which at first appears meaningless, but it's hard to find anything which covers the basics in just getting up and running :-(

@andreievg
Copy link
Collaborator Author

It's been quite the learning experience, but slowly starting to get easier :-) The client/UI side of it will be another set of things to figure out - hopefully there are enough examples to work from, but the problem I often find with docs and examples is that there is a lot of technical detail, which at first appears meaningless, but it's hard to find anything which covers the basics in just getting up and running :-(

Yep, it's been quite apparent that we don't have a direct onboarding material. I was hoping we could have set of tutorials to do the common things (adding schema + repository, adding service + graphql, consuming api with front end and adding new list and detail page). This will happen eventually.

Rather than split things up vertically by areas of functionality, I'd thought it would be easier for me as a newbie to work horizontally

Yeah that sounds great, I was trying to break it down that way, rather then the whole stack vertically.

Once that's done, I'll create sub-branches to add in the temperature logs, breaches and breach configs, and then link in my crate to pull in live data from USB and create the service API to translate it into the DB schema.

I was wondering, to introduce the changes to develop branch, are you planning to make one PR ? I had a look at the branch, it's already 3k lines of code, do you mind if I break it down into some manageable sized PRs and start reviewing them ?

@andreievg
Copy link
Collaborator Author

I've made the crate public

🎉

@andreievg andreievg self-assigned this Aug 14, 2023
@adrianwb
Copy link
Contributor

It's been quite the learning experience, but slowly starting to get easier :-) The client/UI side of it will be another set of things to figure out - hopefully there are enough examples to work from, but the problem I often find with docs and examples is that there is a lot of technical detail, which at first appears meaningless, but it's hard to find anything which covers the basics in just getting up and running :-(

Yep, it's been quite apparent that we don't have a direct onboarding material. I was hoping we could have set of tutorials to do the common things (adding schema + repository, adding service + graphql, consuming api with front end and adding new list and detail page). This will happen eventually.

No problem - it's been useful in a way to do it all manually, as I've gotten to see a lot of the existing code and started to get a handle on how it all works. However, it's a lot of stuff to do for introducing a new table - any way some of it can be parameterised to make it easier in future?

Rather than split things up vertically by areas of functionality, I'd thought it would be easier for me as a newbie to work horizontally

Yeah that sounds great, I was trying to break it down that way, rather then the whole stack vertically.

Once that's done, I'll create sub-branches to add in the temperature logs, breaches and breach configs, and then link in my crate to pull in live data from USB and create the service API to translate it into the DB schema.

I was wondering, to introduce the changes to develop branch, are you planning to make one PR ? I had a look at the branch, it's already 3k lines of code, do you mind if I break it down into some manageable sized PRs and start reviewing them ?

By all means go for it and split it up in whatever way seems useful. Once you've done that, I'll use the same PR structure for the breaches and temperature logs.

The vast majority of those 3k lines is in new modules which have been copy/pasted from the equivalent location ones, so hopefully it won't be too big a job to review those - they just need to be sanity checked to make sure that I've not missed something or misunderstood what it was supposed to be doing. There's also some bits of code deliberately commented out for now as they don't apply for the sensor table itself (e.g. sensor in use checks - there are no related records yet) but which will be needed when I add in breaches and temperature logs.

From what I've been able to tell so far, sensors are syncing with 4D as expected (I've used DB Browser for SQLite to directly edit/add records in the local DB), and I've been able to query them via graphql as well. I've also managed to build and run the client on Windows (thanks @raviSussol !), but no joy in trying to cross-compile the server for Android, either from Windows or from my old Macbook (which won't even compile the server for Mac - probably just too old!).

@andreievg
Copy link
Collaborator Author

@adrianwb,

I couldn't quite break things down and preserve commits, so ended up reviewing the whole lot: #2098

The vast majority of those 3k lines is in new modules which have been copy/pasted from the equivalent location ones, so hopefully it won't be too big a job to review those

Yep, indeed it wasn't, was very straight forward

they just need to be sanity checked to make sure that I've not missed something or misunderstood what it was supposed to be doing

I got a bit confused about the mutations that were added, I made a comment in PR

From what I've been able to tell so far, sensors are syncing with 4D as expected

🎉, that's great, and the sync code + tests look great.

However, it's a lot of stuff to do for introducing a new table - any way some of it can be parameterised to make it easier in future

Although I agree that it's a bit of code, but I am not sure if it's that time consuming, I think with proper tutorial it should be simple and shouldn't really take too long (a bit of copy paste and replace). Part of the boilerplate is due to typing, partitioning and breaking things down into layers. Especially annoying is the mapping between different types in layers, this can be improved (i'll link you to the omSupply central server research work, where one type is created for database, graphql and serde (and there is no need for translation and mapping is reduced).

Btw the main reason for service layer, is the ability to test both business logic and mapping (including loaders etc..) independently.

We could actually make it faster to add new tables, but I think it would involve further abstraction and perhaps more macros, which would have negative impact on readability.

Anyways, perhaps you will have some suggestions on how we can make things a bit easier, looking forward to getting further feedback from someone with fresh perspective on the project, thanks !

@adrianwb
Copy link
Contributor

@andreievg FYI I'm back and plan to start working on this again tomorrow...

@andreievg
Copy link
Collaborator Author

@adrianwb, great, thanks!, Btw, I haven't done any further work on this, while you were away

@adrianwb
Copy link
Contributor

I've been selected for jury service, starting on Monday. Chances are that I won't be picked as a juror but if I do, then I could be out of action for a while, depending on the length of the trial :-(

I've mostly got #2307 working, but it's not finished yet. Hopefully there's enough there to build on...

@andreievg
Copy link
Collaborator Author

I think we can safely close this issue, there are a couple of carry overs but they are captured in other issues and not super urgent, thanks @adrianwb for all the work on this so far, it's awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic feature: coldchain needs architecture/solution Needs wider dev input on general solution
Projects
None yet
Development

No branches or pull requests

4 participants