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

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

Merged
merged 20 commits into from Mar 24, 2016

Conversation

@pazaan
Copy link
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.

Goedhart, Lennart added 10 commits Dec 29, 2015
Update unit tests to work with new device. Modifications to verifyChecksum to avoid the trim()
…eLink has no data for the downloaded time period.
 into pazaan/bayer-contour-next-astm
 into pazaan/bayer-contour-next-astm
this.valid = false;
this.frame_len = 0;
this.payload = null;
return this;

This comment has been minimized.

Copy link
@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.

Copy link
@pazaan

pazaan Jan 27, 2016

Author 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.

Copy link
@gniezen
hostID : 0x42, // B
deviceID: 0x43 // C
};
var MAGIC_HEADER = 'ABC';

This comment has been minimized.

Copy link
@gniezen

gniezen Jan 21, 2016

Member

Much better! :)

var getOneRecord = function (data, callback) {
var cmd = buildAckPacket();
bcnCommandResponse(cmd, function (err, record) {
// TODO - add error handling

This comment has been minimized.

Copy link
@gniezen

gniezen Jan 21, 2016

Member

What error handling still needs to be done here?

This comment has been minimized.

Copy link
@pazaan

pazaan Jan 27, 2016

Author Member

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

pkt.parsed_payload = commandpacket.parser(pkt);
rawtext += pkt.parsed_payload;
}
var debugFrame = function (astmFrame) {

This comment has been minimized.

Copy link
@gniezen

gniezen Jan 21, 2016

Member

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

// 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.

Copy link
@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.

Copy link
@pazaan

pazaan Jan 27, 2016

Author 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" ;)

var abortTimer = setTimeout(function () {
clearInterval(listenTimer);
debug('TIMEOUT');
callback('TIMEOUT', null);
callback('TIMEOUT');

This comment has been minimized.

Copy link
@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.

Copy link
@pazaan

pazaan Jan 27, 2016

Author 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.

Copy link
@gniezen

gniezen Jan 27, 2016

Member

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


if(packetHead['HEADER'] != MAGIC_HEADER){
debug('Invalid packet from Contour device');
callback('INVALID_USB_PACKET');

This comment has been minimized.

Copy link
@gniezen

gniezen Jan 21, 2016

Member

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

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.

Copy link
@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.

Copy link
@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.

Copy link
@pazaan

pazaan Jan 31, 2016

Author Member

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

}, 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.

Copy link
@gniezen

gniezen Jan 21, 2016

Member

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

This comment has been minimized.

Copy link
@pazaan

pazaan Jan 27, 2016

Author 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 3 commits Jan 30, 2016
…ther than when parsing records.

Rename variables to be more correct.
Add some of the Gerrit's change requests.
Make parseHeader consistent with parseDataRecord.

if(packetHead['HEADER'] !== MAGIC_HEADER){
debug('Invalid packet from Contour device');
callback(new Error('Invalid USB packet received.'), null);

This comment has been minimized.

Copy link
@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.

function () { return (recordType !== 'L' && !error); },
function (callback) {
getOneRecord(data, function (err, result) {
recordType = result.recordType;

This comment has been minimized.

Copy link
@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.

pazaan added 2 commits Feb 4, 2016
@gniezen
Copy link
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.

pazaan added 2 commits Mar 19, 2016
 into pazaan/bayer-contour-next-astm
@gniezen
Copy link
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
Copy link
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
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.