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

Add initial support for Casio GB-6900B #1377

Merged
merged 5 commits into from Jan 6, 2019

Conversation

@andyboeh
Copy link
Contributor

commented Dec 28, 2018

This PR adds initial support for the Casio GB-6900B watch. It was tested only on GB-X6900B (the only one I have), but it should work on some other models (GB-6900B, GB-5600B) as well. Other models might be easy to add (e.g. GBA-400), but I haven't got any other Casio devices to test with.

The watch has the ability to save power by turning off bluetooth after some inactivity and turn it back on afterwards. However, this is completely untested.

I'm not entirely sure if I got the Gatt server with the threading right, since I'm not very familiar with Android development.

Looking forward to your comments!

@cpfeiffer

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2018

Thanks a lot for your contribution. I will have a look at it shortly.

To get this right license-wise, I'm logging my findings here (please correct me if I'm wrong):
This is based on https://play.google.com/store/apps/details?id=com.bluewatcher which is a discontinued app and whose source code is available here: https://github.com/masterjc/bluewatcher (just a single commit). It's licensed under the LGPL-3.0.

Mixing LGPL-3.0 code (Bluewatcher) with AGPL code (Gadgetbridge) is allowed, but AFAIU, the original LGPL code would first need to be converted to GPL code and can then be mixed with (Gadgetbridge's) AGPL code (see here: https://opensource.stackexchange.com/questions/5637/use-lgpl-code-in-an-agpl-project). The resulting "mixed" source code would then be partly licensed under the GPL and partly under the AGPL. This is not really possible in its current form because the code was at most copied in parts and taken as an inspiration for own development.

The original auther Juan Carlos Fernández Jara @masterjc states

  • FOR DEVELOPERS: If you want to access BlueWatcher's source code and continue developing/maintaining it, please, contact us. Remember Casio never helped us. All BlueWatcher source code and Bluetooth protocol has been made by our own work.

So I'll try to contact him and ask for his opinion on how to proceed.

Copy link
Contributor

left a comment

Wow, nice job! ❤️

{

}
}

This comment has been minimized.

Copy link
@cpfeiffer

cpfeiffer Dec 29, 2018

Contributor

AFAICS the thread doesn't really do anything?

This comment has been minimized.

Copy link
@andyboeh

andyboeh Dec 29, 2018

Author Contributor

Right. I tried running the server without this thread, but then the client communication stalls. I suppose that the thread is terminated when run() finishes, that's why I added the calls to sleep(). I should've added a way to stop the thread, though.

This comment has been minimized.

Copy link
@cpfeiffer

cpfeiffer Jan 2, 2019

Contributor

Yes, the thread should terminate together with the *DeviceSupport instance. I guess from there you should call the new quit() method? BTW, for calling wait() you need to synchronize on this (see the method's javadoc).

And where did you get the time to wait from (100ms)? Waking up every 100ms sounds a bit battery intensive to me.

For the gatt client (which most other devices use), we use the BtLEQueue to handle all gatt interactions in a separate thread with proper synchronization and ordering. Maybe we need the same for gatt servers!?

Do you have some time and energy to investigate how the gatt server communication is supposed to work?

  • with proper synchronization
  • without waiting/polling

I don't want to scare you away with such a big task -- I'm really happy about your contribution already!

The thing is that getting the lowlevel communication right is essential to getting a reliable connection and maintaining low resource usage, considering different phones, different OS versions, different BLE drivers, and especially different gadgets. Even more so because such code tends to get "reused" elsewhere sooner or later.

This comment has been minimized.

Copy link
@andyboeh

andyboeh Jan 3, 2019

Author Contributor

I'm going to use an object to wait and increase the wait time to a minute or so. The 100ms were based on the time it then takes until the thread quits if it's just checking the flag. With the object, I can notify the thread prematurely. I'll push the updated code as soon as it's ready.

Regarding the queue: IMHO that's a separate task after the PR has been accepted. I'm not sure how much time I can invest in the coming weeks, but I'm quite positive that I can find some spare time for this.

Should we create an issue to discuss the implementation options? I'm sure I'll have some more questions on the inner workings of Gadgetbridge.

This comment has been minimized.

Copy link
@cpfeiffer

cpfeiffer Jan 6, 2019

Contributor

Yeah, I'm fine with doing this separately. I'll open a ticket for this.


builder.write(getCharacteristic(CasioGB6900Constants.ALERT_CHARACTERISTIC_UUID), msg);
LOG.info("Showing notification, title: " + title + " message (not sent): " + message);
performConnected(builder.getTransaction());

This comment has been minimized.

Copy link
@cpfeiffer

cpfeiffer Dec 29, 2018

Contributor

This should probably be performInitialized(), since it is usually not possible to do anything sensible before a device is initialized. Gadgetbridge will take care not to initilize a device more than once (based on the GBDevice state).

This comment has been minimized.

Copy link
@andyboeh

andyboeh Dec 29, 2018

Author Contributor

As far as I understood the logic, it works the following:

  • Create a new transaction using performInitialized, this makes sure the device is initialized
  • Add actions to the builder
  • Run the builder using performConnected
TransactionBuilder builder = performInitialized("showNotification");
builder.write(....);
performConnected(builder.getTransaction());

Am I missing something here?

This comment has been minimized.

Copy link
@cpfeiffer

cpfeiffer Jan 2, 2019

Contributor

I'm going to add some javadoc to these methods and also change the HPlusSupport class, which may be the origin of this (I've seen this in a couple of DeviceSupport classes now).

Until I've done this, here's how it works:

  • the TransactionBuilder is used to collect a number of BLE interaction sequences
  • once the interaction sequence is complete, one lets the builder create a Transactionwith all the individual reads, writes, notifys, etc.
  • this Transaction will get executed by BtLEQueue, which takes care of performing the reads, writes etc. in the correct order with the proper synchronization.

The performXXX() methods are just a way to perform something before the transaction is executed. I.e. performConnected() will make sure that the device is connected before running the transaction.

performInitialized() will take care that the device is not only connected, but that the whole initialization is already done. The reason is "autoconnect". If you have a device that is not connected, and you receive a message (e.g. a mail, sms, ...), you need to first connect to the device, perform the entire initialization sequence and only then send the message to the device for display, vibration or whatever.

So all you do is

TransactionBuilder builder = performInitialized("showNotification");
builder.write(....);
builder.queue(getQueue());

If you want to interact with the device without sending the initialization sequence in advance (e.g. because your are part of the initialization sequence), you may use performConnected().

performImmediately() is a very special thing and rarely needed. It will bypass the ordering of the currently executed transaction in order to perform some BLE interactions immediately.

This comment has been minimized.

Copy link
@andyboeh

andyboeh Jan 3, 2019

Author Contributor

Oh, i see, thanks for clarifying the situation! Indeed, I took parts of the code from other device support classes, but I don't remember which one, since I went through most of them.

In this case, performImmediately() shouldn't be needed. I'll push the updated code soon.

arr[i+3] = bInfo[i];
}
builder.write(getCharacteristic(CasioGB6900Constants.MORE_ALERT_FOR_LONG_UUID), arr);
performConnected(builder.getTransaction());

This comment has been minimized.

Copy link
@cpfeiffer

cpfeiffer Dec 29, 2018

Contributor

Same here.

@andyboeh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2018

The comments should be resolved now, apart from the two where I replied directly - thanks for the thorough review!

…lized(), correctly terminate threads
@andyboeh

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

Apart from (hopefully) fixing the remaining issues, I added a handler thread to poll the Tx power level every minute - the GShock+ app does that as well (for no apparent reason). The value reported is always 0, though.

Copy link
Contributor

left a comment

Great job, I think this is ready for merging 👍 There's a few remaining performConnected() calls that can be replaced with builder.queue(getQueue()).

I've got one question for the device recognition based on name.startsWith("CASIO"). Is there a better way to detect these devices, e.g. based on advertising services? I know we use the name for detecting many devices, but we try to only do this in the last resort, when nothing else works. It also bit us at least once (see #1339).

{

}
}

This comment has been minimized.

Copy link
@cpfeiffer

cpfeiffer Jan 6, 2019

Contributor

Yeah, I'm fine with doing this separately. I'll open a ticket for this.

@cpfeiffer cpfeiffer merged commit cf870bf into Freeyourgadget:master Jan 6, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andyboeh

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

Thanks for applying - the remaining performConnected() calls are part of the initialization sequence (I should have documented that):

  • Notify a few characteristics
  • The watch asks for the current time via a notification
  • After successfully applying the time, the watch responds again with a notification which triggers the device initialized state
@andyboeh andyboeh deleted the andyboeh:device_casio branch May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.