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

mSupply Integration: Extract deviceType into separate field #211

Closed kat-ms closed 4 months ago

kat-ms commented 2 years ago

Describe the bug See the related Desktop issue for the description and repro steps here: https://github.com/sussol/msupply/issues/9979

Currently the deviceType is embedded into the MAC Address. The mobile integration works ok, but the CCE integration doesn't, due to MAC Address validation on the server side.

To Reproduce Steps to reproduce the behaviour:

  1. Set up any new BT sensor and sync to mSupply
  2. Look in mSupply for the sensor (Item -> Vaccines -> Sensors) on the relevant store, or use record browser
  3. Whoops - no sensor

Expected behavior Should be a sensor

Smartphone (please complete the following information):

Additional context There is a related mobile issue here: https://github.com/openmsupply/mobile/issues/4524

chetstone commented 2 years ago

Extracting the deviceType into a separate field is one way to address the issue, but there may be an alternative that merits investigation. When I started adding the Laird sensor, I assumed that react-native-ble-plx#discoverAllServicesAndCharacteristicsForDevice would actually return what its name implies, the characteristics of the device, which would be enough information to determine the type of device. I was surprised that it returned no useful information.  I discussed this with Josh, and he looked into the code of react-native-ble-plx, or rather its underlying Android library, and confirmed that this was the case. It seems likely that this library, or at least some lower level bluetooth library, would either cache this information to make it possible to communicate with the device, or would discover it when it connects, and it's just a deficiency in the API that it doesn't pass it up to the caller. So maybe it would be possible to enhance that API to get the information, so it would not be necessary to store it in msupply-cold-chain (or mobile's) database. At the time I discussed it with Josh, he didn't think we should get into modifying those lower-level components, and we decided to take the approach I implemented. Maybe it's time to take another look. via Newton MailOn Thu, Dec 09, 2021 at 1:55pm, Kat @.> wrote:Describe the bug See the related Desktop issue for the description and repro steps here: sussol/msupply#9979To Reproduce Steps to reproduce the behaviour:Set up any new BT sensor and sync to mSupplyLook in mSupply for the sensor (Item -> Vaccines -> Sensors) on the relevant store, or use record browserWhoops - no sensorExpected behavior Should be a sensorSmartphone (please complete the following information):Device: N/AOS: N/ABrowser N/AVersion 0.5.0-rc0Additional context There is a related mobile issue here: openmsupply/mobile#4524—You are receiving this because you are subscribed to this thread.Reply to this email directly, view it on GitHub, or unsubscribe.[ { @.": "schema.org", @.": "EmailMessage", "potentialAction": { @.": "ViewAction", "target": "github.com/openmsupply/msupply-cold-chain/issues/211", "url": "github.com/openmsupply/msupply-cold-chain/issues/211", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { @.***": "Organization", "name": "GitHub", "url": "github.com" } } ]

kat-ms commented 2 years ago

Thanks @chetstone - I agree that having the lower-level library surfacing the device type would be more ideal - having to persist it and then retrieve it again between calls is a bit cumbersome. I also think though, that despite its current inefficiency, there is still some benefit in saving / passing the device type upstream from a reporting perspective, as being able to centrally identify how many of a particular type of sensor exists at any given location could be potentially useful?

That being said, persisting the deviceType doesn't necessarily preclude fixing the library 😄 ...although it does probably lower the priority on it a bit...

mark-prins commented 4 months ago

post triage - decided not to do