Refactor Bayer Contour ASTM driver. Add support for Contour NEXT LINK 2.4 #221

Merged
merged 20 commits into from Mar 24, 2016

Conversation

Projects
None yet
2 participants
@pazaan
Member

pazaan commented Jan 7, 2016

I agree to the terms of Tidepool Project’s Volunteer/Contributor License Agreement v1.0
as it exists at http://tidepool-org.github.io/TidepoolVCLA.pdf on 7th January 2016.

lib/drivers/bayerContourNext.js
+ this.valid = false;
+ this.frame_len = 0;
+ this.payload = null;
+ return this;

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

Is it necessary to return here, as we don't seem to be using the returned value?

@gniezen

gniezen Jan 21, 2016

Member

Is it necessary to return here, as we don't seem to be using the returned value?

This comment has been minimized.

@pazaan

pazaan Jan 27, 2016

Member

We are using it, further down.
var astmFrameBuffer = {
...
}.reset();
It's like a constructor of sorts to avoid code duplication.

@pazaan

pazaan Jan 27, 2016

Member

We are using it, further down.
var astmFrameBuffer = {
...
}.reset();
It's like a constructor of sorts to avoid code duplication.

This comment has been minimized.

@gniezen

gniezen Jan 27, 2016

Member

👍

- hostID : 0x42, // B
- deviceID: 0x43 // C
- };
+ var MAGIC_HEADER = 'ABC';

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

Much better! :)

@gniezen

gniezen Jan 21, 2016

Member

Much better! :)

lib/drivers/bayerContourNext.js
+ var getOneRecord = function (data, callback) {
+ var cmd = buildAckPacket();
+ bcnCommandResponse(cmd, function (err, record) {
+ // TODO - add error handling

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

What error handling still needs to be done here?

@gniezen

gniezen Jan 21, 2016

Member

What error handling still needs to be done here?

This comment has been minimized.

@pazaan

pazaan Jan 27, 2016

Member

Quite right, it should be done in bcnCommandResponse. This is a redundant comment.

@pazaan

pazaan Jan 27, 2016

Member

Quite right, it should be done in bcnCommandResponse. This is a redundant comment.

lib/drivers/bayerContourNext.js
- pkt.parsed_payload = commandpacket.parser(pkt);
- rawtext += pkt.parsed_payload;
- }
+ var debugFrame = function (astmFrame) {

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

I guess if we're not using this function anymore it's better to remove it.

@gniezen

gniezen Jan 21, 2016

Member

I guess if we're not using this function anymore it's better to remove it.

lib/drivers/bayerContourNext.js
+ // We're using the hidDevice as a message/frame buffer, not a USB packet buffer
+ // to save on buffer copying. It could be refactored differently easily enough.
+ var frame = hidDevice.nextPacket();
+ // TODO - we should ideally check the validity of the ASTM frame (sequence number, checksum etc) here as well for retries

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

I thought we are verifying the checksum, or is there a separate checksum for the ASTM frame itself?

@gniezen

gniezen Jan 21, 2016

Member

I thought we are verifying the checksum, or is there a separate checksum for the ASTM frame itself?

This comment has been minimized.

@pazaan

pazaan Jan 27, 2016

Member

According to the Contour ASTM protocol doc, if a sequence number is out of order, or the checksum doesn't match at this point, we should send a NAK. At the moment, we're always just sending an ACK, and not caring about the sequence number. This doesn't seem like too much of a hassle with USB devices, but could be a problem later if we want to use this code for serial devices (where interference and retransmits are more of a factor).

We're validating the checksum only after we've done the communication with the USB device (after we've pushed the results onto our record array). I'll have a look and see how much work is involved in doing it "properly" ;)

@pazaan

pazaan Jan 27, 2016

Member

According to the Contour ASTM protocol doc, if a sequence number is out of order, or the checksum doesn't match at this point, we should send a NAK. At the moment, we're always just sending an ACK, and not caring about the sequence number. This doesn't seem like too much of a hassle with USB devices, but could be a problem later if we want to use this code for serial devices (where interference and retransmits are more of a factor).

We're validating the checksum only after we've done the communication with the USB device (after we've pushed the results onto our record array). I'll have a look and see how much work is involved in doing it "properly" ;)

lib/drivers/bayerContourNext.js
var abortTimer = setTimeout(function () {
clearInterval(listenTimer);
debug('TIMEOUT');
- callback('TIMEOUT', null);
+ callback('TIMEOUT');

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

To have this surface properly in the UI, should be changed to callback(new Error('Timeout error.'),null);

@gniezen

gniezen Jan 21, 2016

Member

To have this surface properly in the UI, should be changed to callback(new Error('Timeout error.'),null);

This comment has been minimized.

@pazaan

pazaan Jan 27, 2016

Member

Will update. Do you also want me to modify all the other callback(err, null) in the existing code?

@pazaan

pazaan Jan 27, 2016

Member

Will update. Do you also want me to modify all the other callback(err, null) in the existing code?

This comment has been minimized.

@gniezen

gniezen Jan 27, 2016

Member

If they're in the callback(err, null); format they are correct.

@gniezen

gniezen Jan 27, 2016

Member

If they're in the callback(err, null); format they are correct.

lib/drivers/bayerContourNext.js
+
+ if(packetHead['HEADER'] != MAGIC_HEADER){
+ debug('Invalid packet from Contour device');
+ callback('INVALID_USB_PACKET');

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

Rather callback(new Error('Invalid USB packet'), null);?

@gniezen

gniezen Jan 21, 2016

Member

Rather callback(new Error('Invalid USB packet'), null);?

lib/drivers/bayerContourNext.js
+ callback(null);
+ }
+ });
+ }, 20); // The Contour timeout is 15 seconds, but let's be slow enough to be kind to the CPU.

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

Maybe make this comment a bit clearer that the 20 is in milliseconds, not seconds?

@gniezen

gniezen Jan 21, 2016

Member

Maybe make this comment a bit clearer that the 20 is in milliseconds, not seconds?

@@ -416,7 +441,7 @@ module.exports = function (config) {
}
dataToPost.push(smbg);
}
- }else{
+ } else {
debug('Device has no records to upload');
throw(new Error('Device has no records to upload'));

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

can we return this in a callback so that it will surface properly in the UI?

@gniezen

gniezen Jan 21, 2016

Member

can we return this in a callback so that it will surface properly in the UI?

This comment has been minimized.

@pazaan

pazaan Jan 31, 2016

Member

There's a try/catch around the calling function now, so this exception will be surfaced in the UI.

@pazaan

pazaan Jan 31, 2016

Member

There's a try/catch around the calling function now, so this exception will be surfaced in the UI.

lib/drivers/bayerContourNext.js
}, 20);
}
- async.timesSeries(data.nrecs, getOneRecordWithProgress, function(err, result) {
+ // FIXME - We're adding +1 to skip the Patient Record (P)

This comment has been minimized.

@gniezen

gniezen Jan 21, 2016

Member

I don't quite follow what is going on here?

@gniezen

gniezen Jan 21, 2016

Member

I don't quite follow what is going on here?

This comment has been minimized.

@pazaan

pazaan Jan 27, 2016

Member

The first record that the Contour device gives back is a Patient record 'P'. The rest are data record types 'R'. I'll update the data structure to make it more obvious.

@pazaan

pazaan Jan 27, 2016

Member

The first record that the Contour device gives back is a Patient record 'P'. The rest are data record types 'R'. I'll update the data structure to make it more obvious.

pazaan added some commits Jan 30, 2016

Refactor to do the checksum verification at the ingestion of data, ra…
…ther than when parsing records.

Rename variables to be more correct.
Add some of the Gerrit's change requests.
Fix up some indenting.
Make parseHeader consistent with parseDataRecord.
lib/drivers/bayerContourNext.js
+
+ if(packetHead['HEADER'] !== MAGIC_HEADER){
+ debug('Invalid packet from Contour device');
+ callback(new Error('Invalid USB packet received.'), null);

This comment has been minimized.

@gniezen

gniezen Feb 1, 2016

Member

I was just testing the error handling by unplugging the device during an upload. You need to return the callback, otherwise it will still try to process and upload the invalid data. Also, remember to clear the timeout timer and interval timers.

@gniezen

gniezen Feb 1, 2016

Member

I was just testing the error handling by unplugging the device during an upload. You need to return the callback, otherwise it will still try to process and upload the invalid data. Also, remember to clear the timeout timer and interval timers.

lib/drivers/bayerContourNext.js
+ function () { return (recordType !== 'L' && !error); },
+ function (callback) {
+ getOneRecord(data, function (err, result) {
+ recordType = result.recordType;

This comment has been minimized.

@gniezen

gniezen Feb 1, 2016

Member

If getOneRecord returns an error, this code will still try to process result first before handling the error. Also, remember to do a return callback(err,null), with just a callback(err,null) it will still try to process and upload the invalid data.

@gniezen

gniezen Feb 1, 2016

Member

If getOneRecord returns an error, this code will still try to process result first before handling the error. Also, remember to do a return callback(err,null), with just a callback(err,null) it will still try to process and upload the invalid data.

@gniezen

This comment has been minimized.

Show comment
Hide comment
@gniezen

gniezen Feb 26, 2016

Member

Hi there! I'm sorry I didn't get a chance to merge this before going on holiday - this PR will now need a little bit of updating to be compatible with the redux work that got merged into master recently. The main change is that there is now a single device definition in redux/reducers/devices.js where previously the definitions were all over the place.

Member

gniezen commented Feb 26, 2016

Hi there! I'm sorry I didn't get a chance to merge this before going on holiday - this PR will now need a little bit of updating to be compatible with the redux work that got merged into master recently. The main change is that there is now a single device definition in redux/reducers/devices.js where previously the definitions were all over the place.

@gniezen

This comment has been minimized.

Show comment
Hide comment
@gniezen

gniezen Mar 21, 2016

Member

Thanks! I've successfully uploaded (and verified BG readings) of:

  • Bayer Contour Next USB
  • Bayer Contour Next
  • Bayer Contour USB
  • Bayer Contour Next Link

One that still need to be fixed after the merge: lib/core/deviceInfo.js and lib/state/appState.js has both been deprecated and were removed from master. Don't put them back! ;)

Member

gniezen commented Mar 21, 2016

Thanks! I've successfully uploaded (and verified BG readings) of:

  • Bayer Contour Next USB
  • Bayer Contour Next
  • Bayer Contour USB
  • Bayer Contour Next Link

One that still need to be fixed after the merge: lib/core/deviceInfo.js and lib/state/appState.js has both been deprecated and were removed from master. Don't put them back! ;)

@gniezen

This comment has been minimized.

Show comment
Hide comment
@gniezen

gniezen Mar 24, 2016

Member

LGTM! Will try to merge as soon.

Member

gniezen commented Mar 24, 2016

LGTM! Will try to merge as soon.

@gniezen gniezen merged commit e08b943 into tidepool-org:master Mar 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pazaan pazaan deleted the pazaan:pazaan/bayer-contour-next-astm branch Mar 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment