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

Binary protocol over bluetooth #82

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@raduiliescu
Contributor

raduiliescu commented Mar 15, 2015

Change data exachange over bluetooth to a binary format.

Advantages:

  1. data can easily be decoded;
  2. more robust to error and is not taking just parts of the packets;
  3. can be easily extended:
  • for example I want to add next getMs and a packet id to the packets; the bluetooth chip is buffering packets if here is no connection and packets can be get with read.

raduiliescu added some commits Feb 26, 2015

binary_protocol: add support for unpacking binary data
add support to extract int16, uint16 and uint8 from a byte array
java doesn't have support for unsigned
this is required for having binary, reliable & extensible
communication protocol between wixel and android
binary_protocol: add a placer for binary packet info
currently it contains only DATA_PACKET type
binary_protocol: decode binary data packet
add support for decoding binary data packets
required for having binary reliable and extensible communication protocol
between wixel and android
binary_protocol: add support for binary protocol on ble bluetooth
create a packet read state machine DexCollectionServices
1. read packet type
2. read len
3. do read until len bytes are read
if packet is malformed (unknown, an invalid len, too long or short),
the state machine will be restarted

tested with all above malformed cases;
tested with a mix of random bad/good packets.
@@ -350,18 +362,86 @@ public void setCharacteristicNotification(BluetoothGattCharacteristic characteri
}
public void setSerialDataToTransmitterRawData(byte[] buffer, int len) {

This comment has been minimized.

@StephenBlackWasAlreadyTaken

StephenBlackWasAlreadyTaken Mar 17, 2015

Owner

First off, this PR is super cool, my biggest issue is that I dont want an app update to make someones currently functional wixel stop being able to communicate with xDrip, can we split this setSerialDataToTransmitterRawData and have it go down two logical paths, one if its the old style, one if its the new style,
Also I would love it if the start of each of these new packets was a version number (this being version 1, that way if we down the road add in some more params we can easily check which version of the a transmitter data parser to use)

@StephenBlackWasAlreadyTaken

StephenBlackWasAlreadyTaken Mar 17, 2015

Owner

First off, this PR is super cool, my biggest issue is that I dont want an app update to make someones currently functional wixel stop being able to communicate with xDrip, can we split this setSerialDataToTransmitterRawData and have it go down two logical paths, one if its the old style, one if its the new style,
Also I would love it if the start of each of these new packets was a version number (this being version 1, that way if we down the road add in some more params we can easily check which version of the a transmitter data parser to use)

This comment has been minimized.

@raduiliescu

raduiliescu Mar 20, 2015

Contributor

Have added a fallback mechanism see line 397

@raduiliescu

raduiliescu Mar 20, 2015

Contributor

Have added a fallback mechanism see line 397

@saercnap

This comment has been minimized.

Show comment
Hide comment
@saercnap

saercnap Mar 17, 2015

Contributor

Consider using something like protocol buffers [1] or thrift [2] instead of manually versioning the binary protocol

[1] https://developers.google.com/protocol-buffers/docs/overview
[2] https://thrift.apache.org/

Contributor

saercnap commented Mar 17, 2015

Consider using something like protocol buffers [1] or thrift [2] instead of manually versioning the binary protocol

[1] https://developers.google.com/protocol-buffers/docs/overview
[2] https://thrift.apache.org/

@raduiliescu

This comment has been minimized.

Show comment
Hide comment
@raduiliescu

raduiliescu Mar 18, 2015

Contributor

In general I have some issues with keeping backwords compatibility. Can be a big pain on new development.
In my opinion, xDrip is in a begining phase, so I wouldn't care too much for backword compatibility.
It still involves a lot of DIY and everyone needs to compile the code and install it. Who upgrades the xDrip app doesn't have an issue in upgrading the wixel one.

On versioning I see two possibilities:

  1. start with the assertion that xDrip app will support and be backwords compatible with all wixel versions.
    Putting code for this will complicate the things and in this case I'll jump on protobuf. The library might be a bit too big for wixel.
    I've never used it in Java and one issue I see is getting the buffer from notify and feeding the google proto deserializer. Need to know how much to read, because on a write to uart bluetooth can split the packets.
    It will increase a bit the wixel code and will present a problem on where to put/compile the .proto file. Maybe in a different branch and add the protobuf library and compiled C/JAVA output code in wixel-xDrip/ xDrip.
  2. keep it simple - put the version in the packet and use it just for checks.
    If the version is what we expected then take the packet, otherwise drop it and signal the users that the wixel software is incompatible.

Please let me know of your thoughts.

Contributor

raduiliescu commented Mar 18, 2015

In general I have some issues with keeping backwords compatibility. Can be a big pain on new development.
In my opinion, xDrip is in a begining phase, so I wouldn't care too much for backword compatibility.
It still involves a lot of DIY and everyone needs to compile the code and install it. Who upgrades the xDrip app doesn't have an issue in upgrading the wixel one.

On versioning I see two possibilities:

  1. start with the assertion that xDrip app will support and be backwords compatible with all wixel versions.
    Putting code for this will complicate the things and in this case I'll jump on protobuf. The library might be a bit too big for wixel.
    I've never used it in Java and one issue I see is getting the buffer from notify and feeding the google proto deserializer. Need to know how much to read, because on a write to uart bluetooth can split the packets.
    It will increase a bit the wixel code and will present a problem on where to put/compile the .proto file. Maybe in a different branch and add the protobuf library and compiled C/JAVA output code in wixel-xDrip/ xDrip.
  2. keep it simple - put the version in the packet and use it just for checks.
    If the version is what we expected then take the packet, otherwise drop it and signal the users that the wixel software is incompatible.

Please let me know of your thoughts.

@StephenBlackWasAlreadyTaken

This comment has been minimized.

Show comment
Hide comment
@StephenBlackWasAlreadyTaken

StephenBlackWasAlreadyTaken Mar 19, 2015

Owner

on versioning:
Considdering the only version people are using right now would automaticly stop working if they updated to this, I say we 100% have to support both for at least some amount of time. there are a few hundred people using it right now who may update their app but be SOL because they dont have a miniusb cable to update their wixel.

adding a library to the wixel will be tough, its very tight on space and bceause its so timing dependant I would like to keep from having to jam anything that happens in the main loop from having to touch xdata

Owner

StephenBlackWasAlreadyTaken commented Mar 19, 2015

on versioning:
Considdering the only version people are using right now would automaticly stop working if they updated to this, I say we 100% have to support both for at least some amount of time. there are a few hundred people using it right now who may update their app but be SOL because they dont have a miniusb cable to update their wixel.

adding a library to the wixel will be tough, its very tight on space and bceause its so timing dependant I would like to keep from having to jam anything that happens in the main loop from having to touch xdata

@raduiliescu

This comment has been minimized.

Show comment
Hide comment
@raduiliescu

raduiliescu Mar 19, 2015

Contributor

Ok. Will implement it to allways be backwords compatible.

Contributor

raduiliescu commented Mar 19, 2015

Ok. Will implement it to allways be backwords compatible.

@StephenBlackWasAlreadyTaken

This comment has been minimized.

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