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

Tzachi libre wifi 2 #418

Merged
merged 6 commits into from
May 11, 2018
Merged

Tzachi libre wifi 2 #418

merged 6 commits into from
May 11, 2018

Conversation

tzachi-dar
Copy link
Contributor

This replaces tzachi libre wifi 1 that worked very well with raspberry pi receivers.

Merging this with current code worked well, but there is one thing that somehow frightening me:
As part of the changes to to tomato.java I have removed one buffer that was sent in tomato.resetTomatoState().
Theoretically, this should not change anything, but as android BT code is so sensitive, I wonder if this does not start a new testing cycle for every phone.
(my fear is that since now there is a 150 ms sleep between each two buffers, a change in number of buffers changes timing ). So jamorham@ if you think that this should be reverted let me know.

In any case I'm starting to check the new code.

… allow getting data from external readers, where time is not immediate.

Signed-off-by: Tzachi Dar <tzachi.dar@gmail.com>

# Conflicts:
#	app/src/main/java/com/eveningoutpost/dexdrip/Models/Tomato.java
Signed-off-by: Tzachi Dar <tzachi.dar@gmail.com>
Signed-off-by: Tzachi Dar <tzachi.dar@gmail.com>
Signed-off-by: Tzachi Dar <tzachi.dar@gmail.com>
@tzachi-dar tzachi-dar mentioned this pull request May 4, 2018
@@ -48,7 +48,7 @@ public static BridgeResponse decodeTomatoPacket(byte[] buffer, int len) {
final BridgeResponse reply = new BridgeResponse();
// Check time, probably need to start on sending
long now = JoH.tsl();
if(now - s_lastReceiveTimestamp > 10*1000) {
if(now - s_lastReceiveTimestamp > 3*1000) {
// We did not receive data in 10 seconds, moving to init state again
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this will never take more than 3 seconds even in really bad RF situations?

Also the comment below still references 10 seconds, should be updated to something more generic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed the comment.
The main reason that we need this change is that now (actually for some time now) there is a retry code after crc errors. That retry takes time after 10 seconds, and that might cause an issue, so I have changed that to be 3 seconds.
The distance between the packets should be ~100ms. So no reason to see things taking more than 3 seconds.

ByteBuffer newFreqMessage = ByteBuffer.allocate(2);
newFreqMessage.put(0, (byte) 0xD1);
newFreqMessage.put(1, (byte) 0x05);
ret.add(newFreqMessage);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the effect of removing this? I thought this enabled the required scheduler. This seems unrelated to adding Wifi support?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, from the tomato standpoint, this code is not needed.
When it comes from the factory, it sends data every 5 minutes. Also when we start a new code we tell it to send every 5 minutes.

That said, since code now sends each buffer in a 150 ms period, this has influence on different androids stack implementations (actually if only one buffer is being sent, the wait will not happen), so I'll revert it and we can think if we are brave enough to change it.

hostStatus.put(key, msg);
hostStatusTime.put(key, (time != -1) ? time : JoH.tsl());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a significant duplication of code. I don't propose any changes at this point, but what are the constraints that prevent us for reusing the existing wifi-reader for this purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the main reason that we were not able to reuse the existing code are:

  1. The need to move to a new protocol (see in the main answer why).
  2. The existing code was returning a list of . Since libre has other fields (see LibreWifiData), one would have to have a third class that both will inherit from. this will make code more complex.
  3. The way multiple points are handled: When reading from the g4 each packet matters. when reading from libre, we can skip older readings. That said, if for example last packet contains something like "no sensor" we need to move to the previous packet. So, libre code has to be different.

Collections.addAll(usesBtWixel, BluetoothWixel, LimiTTer, WifiBlueToothWixel);
Collections.addAll(usesWifi, WifiBlueToothWixel,WifiWixel,WifiDexBridgeWixel, Mock);
Collections.addAll(usesBluetooth, BluetoothWixel, DexcomShare, DexbridgeWixel, LimiTTer, WifiBlueToothWixel, DexcomG5, WifiDexBridgeWixel, LimiTTerWifi);
Collections.addAll(usesBtWixel, BluetoothWixel, LimiTTer, WifiBlueToothWixel, LimiTTerWifi); // Name is misleading here, should probably be using dexcollectionservice
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usesBtWixel is to describe the simple text based protocol used by things like LimiTTer and the classic xDrip device. It doesn't include things like xBridge or blucon, miaomiao etc

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see below where we have a specific set for whether something uses dexcollection service

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I know what got me confused:
The tomato, does not have a type of it's own, rather it was using the limiter name.

So, I guess that there are two solutions here:

  1. Change the line to be:
    Collections.addAll(usesBtWixel, BluetoothWixel, LimiTTer, WifiBlueToothWixel, LimiTTerWifi); // describe
    the simple text based protocol used by things like LimiTTer and the classic xDrip device
    I'll still add the LimiTTerWifi since this is still limiter.
  2. Add a new HashSet usesLimiterBT and add limite, limiterWifi there.

What do you think is the better solution?

@jamorham
Copy link
Collaborator

jamorham commented May 8, 2018

Ok, so really change to tomato behavior within xDrip shouldn't be in a PR for adding wifi libre support. That should be in a separate PR. But what is the reason for that change anyway?

Can you also provide a summary as to what this PR is intended to do. Assume I know nothing about the two new collection methods being introduced.

ByteBuffer newFreqMessage = ByteBuffer.allocate(2);
newFreqMessage.put(0, (byte) 0xD1);
newFreqMessage.put(1, (byte) 0x05);
ret.add(newFreqMessage);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, from the tomato standpoint, this code is not needed.
When it comes from the factory, it sends data every 5 minutes. Also when we start a new code we tell it to send every 5 minutes.

That said, since code now sends each buffer in a 150 ms period, this has influence on different androids stack implementations (actually if only one buffer is being sent, the wait will not happen), so I'll revert it and we can think if we are brave enough to change it.

@@ -48,7 +48,7 @@ public static BridgeResponse decodeTomatoPacket(byte[] buffer, int len) {
final BridgeResponse reply = new BridgeResponse();
// Check time, probably need to start on sending
long now = JoH.tsl();
if(now - s_lastReceiveTimestamp > 10*1000) {
if(now - s_lastReceiveTimestamp > 3*1000) {
// We did not receive data in 10 seconds, moving to init state again
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed the comment.
The main reason that we need this change is that now (actually for some time now) there is a retry code after crc errors. That retry takes time after 10 seconds, and that might cause an issue, so I have changed that to be 3 seconds.
The distance between the packets should be ~100ms. So no reason to see things taking more than 3 seconds.

@@ -2480,6 +2483,9 @@ public void onClick(DialogInterface dialog, int which) {
if(DexCollectionType.isLibreOOPAlgorithm(collector)) {
// Rest of this function deals with initial calibration. Since we currently don't have a way to calibrate,
// And even once we will have, there is probably no need to force a calibration at start of sensor use.
displayCurrentInfo();
// JamorHam, should I put here something like:
// ?? if (screen_forced_on) dontKeepScreenOn();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see if I need to add something like:
if (screen_forced_on) dontKeepScreenOn();

hostStatus.put(key, msg);
hostStatusTime.put(key, (time != -1) ? time : JoH.tsl());
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the main reason that we were not able to reuse the existing code are:

  1. The need to move to a new protocol (see in the main answer why).
  2. The existing code was returning a list of . Since libre has other fields (see LibreWifiData), one would have to have a third class that both will inherit from. this will make code more complex.
  3. The way multiple points are handled: When reading from the g4 each packet matters. when reading from libre, we can skip older readings. That said, if for example last packet contains something like "no sensor" we need to move to the previous packet. So, libre code has to be different.

Signed-off-by: Tzachi Dar <tzachi.dar@gmail.com>
@tzachi-dar
Copy link
Contributor Author

This are the changes that are needed to allow xDrip to read libre data from wifi readers, either directly or using a mongodb.
Like the g4 has an all house coverage mode, now the libre (tomato) has such a mode.
The receivers listen on port 50005 and wait for requests to send their data.
Optionally, they also write the data to a mongodb. This is needed to allow the system to work with a more complicated network environment.

A new version of the protocol is introduced. This version is needed in order to allow improved behavior which includes:

  1. Allow to configure the mac of the remote tomato device from xDrip.
  2. Allow sending all data starting at a specific time. (In the current protocol, the time is being translated to number of readings, but one does not know if this will bring entries that are too old).
  3. Tell receivers not to connect to tomato until a given time. (why is this needed? well, consider receiver-a had a reading on 12:00 and then on 12:05. On 12:08 the child went to another room, and a new receiver has connected to libre, and it will now send the data on 12:08. xDrip will reject the data on 12:08 (as it is too close to 12:05) so next reading will only be on 12:13. Telling the remote device not to connect until 12:10 will stop this problem).
  4. better protocol versions. (allow the device to say what protocols it supports).
  5. Sending device type, and a debug message.

As for changes in tomato code:

@tzachi-dar
Copy link
Contributor Author

(meant to say: As for changes in tomato code: - see above.)

// Make tomato send data every 5 minutes
ByteBuffer newFreqMessage = ByteBuffer.allocate(2);
newFreqMessage.put(0, (byte) 0xD1);
newFreqMessage.put(1, (byte) 0x05);
ret.add(newFreqMessage);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, from the tomato standpoint, this code is not needed.
When it comes from the factory, it sends data every 5 minutes. Also when we start a new code we tell it to send every 5 minutes.

That said, since code now sends each buffer in a 150 ms period, this has influence on different androids stack implementations (actually if only one buffer is being sent, the wait will not happen), so I'll revert it and we can think if we are brave enough to change it.

Copy link
Collaborator

@jamorham jamorham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Merging for wider testing.

Reminder that CollectionServiceStarter needs a major rewrite..

@jamorham jamorham merged commit 3de82bb into master May 11, 2018
@tolot27 tolot27 deleted the tzachi_libre_wifi_2 branch November 5, 2020 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants