feat: add bluetooth backend abstraction & support devices with multiple measurements #432
Conversation
|
That looks great so far! You kept that code really tidy. I will be happy to properly review this once it's ready. I noticed your code style is fairly simmilar to what you see in java. Specifically: In dart comments work a bit differently (e.g references to other classes in square brackets, the first line in the comment is a short summary, details come after a blank line). There is a great doc page about it with notes on how to write helpful comments that can act as a general guideline. I just wanted to give you an heads up about it, as this might come unexpected for someone new to dart. |
… was 99% similar across backends
There was a problem hiding this comment.
I'm sorry for the huge PR, but most changed files are just the new bt backend abstraction, dependency changes and added/fixed tests. Maybe I could have made the PR a bit smaller by not also immediately supporting devices with multiple measurements, but this feature seemed an essential part of the new bluetooth backend and not including that would only have saved 2 files.
I'm sure you are already familiar with Github, but if not please make sure to check Viewed after finishing reviewing a file in the PR :)
Linux is still not yet fully supported due to the missing plugins for storage, but will leave that to a different PR. Reading Bluetooth works fine. Also there is likely an issue with the jsaver dependency as I get warnings in the console about no plugin being available.
Note that I did not test on other platforms then Linux yet. I assume there is a workflow to build test apk's for PR's?
app/lib/features/bluetooth/backend/bluetooth_low_energy/ble_device.dart
Outdated
Show resolved
Hide resolved
app/lib/features/bluetooth/backend/bluetooth_low_energy/ble_device.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks allot for this PR. I added some thoughts to the first few files and tried to answer your questions. It would be great if you could consider them and apply these lessons to other files.
Don't feel discouraged by the amount or density, I tried to point everything out. You found some really great solutions, that I didn't comment on.
In the meantime I will try to implement apk building in PRs so you can better test this on real devices.
app/lib/features/bluetooth/backend/bluetooth_low_energy/ble_device.dart
Outdated
Show resolved
Hide resolved
app/lib/features/bluetooth/backend/bluetooth_low_energy/ble_device.dart
Outdated
Show resolved
Hide resolved
|
As a general note: You can drop the support for the flutter_blue_plus backend. This should drastically simplify the code. |
|
Thanks for the thorough review, good to see you are quite opinionated! Will go through your remarks and fix them ;) I'd rather hold of for now on immediately dropping the fbp code unless we know for sure that we have at least one stable release where bluetooth_low_energy is working well. Also we'd loose testing of the bt backed class through your existing tests & mockable class. So dropping fbp would mean more work as we'd need to add more tests. |
Co-authored-by: derdilla <[email protected]>
|
FWIW: Latest action build is using flutter_blue_plus and is working correctly with one of my devices. It's not yet working with the Omron, not sure why but with fbp that device keeps sending bond requests but after bonding it automatically disconnects which triggers the main BleReadFailure state. Looking into that behaviour still |
|
@pimlie I looked into the ble lines you send and they look like enough, potentially even too much permissions (iirc modern android doesn't need that many location permissions). I only glanced at the rest of their code, but it looks like all permissions are requested at once, which fits the observed behavior of opening the app-settings page. Open an issue if you want, I think that's something the maintainer should at least be aware of. This is however not blocking for us since we can request the permissions ourself (architecture wise both sides have valid arguments). In other news: Due to personal reasons I will have less/no time next week, so sorry if you don't get immediate responses. |
|
@derdilla Hey, just a quick check-in that I have been very busy as well and will try to get back to this later this week, maybe next week. Last status from last week was that even with using flutter_blue_plus there was still weird disconnect behaviour after trying to read a measurement from the device. I'm almost sure that this isn't an issue in the current code because you just retry a couple of times and are not depending on a disconnect from the device itself to stop reading as the current code always ever reads only one measurement and then the code itselfs triggers a disconnect. Not sure yet how that can be solved best 😞 |
|
@pimlie glad to hear my retrying had at least some use. You probably already thought of this, but I just had the idea of "simply" ignoring disconnects/retrying until at least one measurement is read. |
I just tested 4bff687 with the Beurer BM 64 and it offered me all measurements that were on the device to select from (it is apparently of the variety "send all stored measurements after taking a measurement"). I have ideas about usability, but those are probably better suited as feature requests after this PR got merged, since I don't want to blow its scope up more than necessary. |
|
@derdilla Pfff, I spent the last week trying to get a proper docker based dev env to work so I could test this PR with hot reloading and I finally got it working I think. Getting some errors about compiling against SDK 34 instead of 35, had to fix/update gradle version, and now it reports warnings about the NDK version (got 23 but it requires 25 and it wants me to update build.gradle). Not sure if these issues are (automatically) fixable by updating the android config, but it seems that setting up an android dev env is not as simple / straight forward as it appears to be from the github workflows? @5FeetUnder Thanks for testing, and yeah creating a new issue/pr to improve UX should be preferred indeed |
|
@pimlie your description sounds like you are building from a stable flutter, and try to build beta-flutter code. In that case you can either use beta flutter or use the latest android/ and pubspec. files from main branch. |
…bluetooth_input from openening appsettings on non android devices chore: improve some logs/formatting
|
@derdilla Hmm, seems you are right. Thanks for that tip. Can we indicate that correctly in the pubspec maybe? I expected that if we require a minimum version (whether that's stable or beta) that that would be enforced by the pubspec. As I kept having issues with the Omron device, I checked back with v1.8.1 published on FDroid and that version has the same connecting issue as I mentioned above that it keeps trying to bond/pair the device and never is able to connect (note that in the past I have been able to connect to the Omron from an Android device, just not currently from various devices). Cause also when trying to pair from the Android bluetooth settings dialog it also has 'issues'. So the Omron just seems very finicky, not just on Android as on Linux it does work after I first manually pair the device but it often also takes a couple of tries. Maybe the Omron doesn't like switching between devices not very much, not sure. Just as a note, the SBM69 device basically always works flawlessly especially after you first pair the device using the system dialogs. That means I believe this PR is ready again for review |
| Future<void> _chainWaitForDisconnectingStateChange() async { | ||
| if (_state == BluetoothDeviceState.disconnecting) { | ||
| logger.finest('Waiting because device is still disconnecting'); | ||
| await Future.delayed(const Duration(milliseconds: 10)) | ||
| .then((_) => _chainWaitForDisconnectingStateChange()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems like it could cause stack overflows (and might fail when some implementation instantly jumps to disconnected, and log spam, bu I trust you tested these). Is a loop also fine?
| Future<void> _chainWaitForDisconnectingStateChange() async { | |
| if (_state == BluetoothDeviceState.disconnecting) { | |
| logger.finest('Waiting because device is still disconnecting'); | |
| await Future.delayed(const Duration(milliseconds: 10)) | |
| .then((_) => _chainWaitForDisconnectingStateChange()); | |
| } | |
| } | |
| Future<void> _chainWaitForDisconnectingStateChange() async { | |
| while(_state != BluetoothDeviceState.disconnecting) { | |
| logger.finest('Waiting because device is still disconnecting'); | |
| await Future.delayed(const Duration(milliseconds: 10)); | |
| } | |
| } |
yes, technically this implementation isn't a chain. Feel free to change if that bothers you
There was a problem hiding this comment.
I'm actually a little bit conflicted about the need for this logic to be honest, this was basically added to try to get the Omron to work reliably on Android but I currently believe that the Omron might be just faulty on Android. That said I don't think adding this logic does any harm and does make sense, so will rework this a bit more
|
|
||
|
|
||
| void main() { | ||
| testWidgets('should show everything and be interactive', (WidgetTester tester) async { |
There was a problem hiding this comment.
I'm still getting a failure:
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: <1>
Actual: <0>
When the exception was thrown, this was the stack:
#4 main.<anonymous closure> (file:///home/derdilla/Coding/Flutter/blood_pressure_app/app/test/features/bluetooth/ui/measurement_multiple_test.dart:83:5)
<asynchronous suspension>
#5 testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:189:15)
<asynchronous suspension>
#6 TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1032:5)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)
This was caught by the test expectation on the following line:
file:///home/derdilla/Coding/Flutter/blood_pressure_app/app/test/features/bluetooth/ui/measurement_multiple_test.dart line 83
The test description was:
should show everything and be interactive
════════════════════════════════════════════════════════════════════════════════════════════════════
00:22 +131 -3: /home/derdilla/Coding/Flutter/blood_pressure_app/app/test/features/bluetooth/ui/measurement_multiple_test.dart: should show everything and be interactive [E]
Test failed. See exception logs above.
The test description was: should show everything and be interactive
If you don't want to learn the test framework that's fine. Just comment this out and I will do appropriate testing when writing the logic out of the UI.
There was a problem hiding this comment.
This is a new error to me, will have a look. The only errors I had locally in the past were related to the golden's
There was a problem hiding this comment.
@derdilla Any idea how I can get mockito to generate a new hashCode for each mock instance?
Issue is that after your suggestion of using a Set for the device discovery list, this test fails because all mock devices have the same hashCode = 0 so adding them to the set only adds the first device
There was a problem hiding this comment.
Hmm, the issue might be something else. Creating a simple set with mock classes does actually work. It just doesnt work when sinking them through the stream, looking at this again
There was a problem hiding this comment.
@derdilla Fixed it for now by ensuring deviceId are also equal when comparing BluetoothDevice's, not sure why hashCode's are the same for the mocks. Have added a todo/code comments for now as the fix is working fine and shouldnt be an issue. Feel free to investigate further if you have the time and wanna spent that on this, but as its working now I'd rather spent my time on improving other things ;)
@pimlie, that seems reasonable. The CI should probably also run with the oldest supported version. Sorry for the confusion the version caused, I really should get out of the habit of having undocumented things like this. You are the first major contributor so feedback like this is highly appreciated. As soon as this is ready for merge: Do you prefer your PR rebased (main-branch gets 90 nicely named conventional-commits) or squashed (one clean commit and we rely on gh if we ever need details)? |
Co-authored-by: derdilla <[email protected]>
Co-authored-by: derdilla <[email protected]>
Co-authored-by: derdilla <[email protected]>
I always prefer to keep the master branch as clean as possible, no need to pollute the git log with all the stupid debug commits in this branch. So a single squash should be fine. Thanks for asking :) |
Resolves #423
Resolves #352
bluetooth_low_energypackage will be the best (the maintainer is quite responsive tho) so with this abstraction we should be able to quickly switch between backends if needed.logging/loggingpackage. This provide much more granular log levels and would also make it easier to support in-app logging etc if we ever decide we want that (e.g. think debug support in beta builds). (I added this because I wanted to be able to see the bluetooth_low_energy logs which also used logging)