Fix discover services on Windows

rohitsangwan01 commented 1 month ago

Type

enhancement

Description

Changed DiscoverServicesAsync method to improve performance and code readability:
- Transitioned from asynchronous to synchronous execution.
- Directly accessed GATT services and characteristics from a cached map, enhancing efficiency.
- Simplified the parsing of characteristic properties, reducing code complexity.
Updated method signature in the header file to match the implementation changes.

Changes walkthrough

Relevant files

Enhancement

universal_ble_plugin.cpp `Optimize Service Discovery and Characteristic Properties Parsing` windows/src/universal_ble_plugin.cpp Changed `DiscoverServicesAsync` from `winrt::fire_and_forget` to `void`. Optimized service discovery by utilizing `bluetoothDeviceAgent.gatt_map_` directly. Streamlined characteristic properties parsing and removed redundant code.	+40/-54
universal_ble_plugin.h `Update DiscoverServicesAsync Signature` windows/src/universal_ble_plugin.h Updated `DiscoverServicesAsync` signature to reflect the change from `winrt::fire_and_forget` to `void`.	+1/-1

✨ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent[bot] commented 1 month ago

PR Description updated to latest commit (https://github.com/Navideck/universal_ble/commit/1eea38175874085b27c795fb1a81683b1a0e6387)

codiumai-pr-agent[bot] commented 1 month ago

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are focused on a specific functionality (DiscoverServicesAsync method) with a clear objective: improving performance and code readability by transitioning from asynchronous to synchronous execution and optimizing service discovery. The changes are well-contained within two files, making it easier to review.
🧪 Relevant tests	No
🔍 Possible issues	Possible Regression: Changing from asynchronous (`winrt::fire_and_forget`) to synchronous execution might introduce blocking behavior if not handled properly, especially in UI applications where responsiveness is critical. It's important to ensure that this change does not negatively impact the user experience by inadvertently blocking the UI thread.
🔍 Possible issues	Error Handling: The removal of asynchronous execution and the associated `co_await` and `co_return` patterns simplifies the code but also changes how errors are handled. It's crucial to verify that all error scenarios are still appropriately managed, especially since the original asynchronous implementation might have provided built-in mechanisms for error handling that are now handled differently.
🔒 Security concerns	No

✨ Review tool usage guide:

**Overview:** The `review` tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be [added](https://pr-agent-docs.codium.ai/tools/review/#general-configurations) by configuring the tool. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on any PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L23) related to the review tool (`pr_reviewer` section), use the following template: ``` /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_reviewer] some_config1=... some_config2=... ``` See the review [usage page](https://pr-agent-docs.codium.ai/tools/review/) for a comprehensive guide on using this tool.

codiumai-pr-agent[bot] commented 1 month ago

PR Code Suggestions

Category	Suggestions
Maintainability	Simplify repetitive `if` conditions with a `switch` statement or a map. ___ Consider using a `switch` statement or a map to simplify the repetitive `if` conditions for setting properties. This will make the code cleaner and more maintainable. [windows/src/universal_ble_plugin.cpp [1100-1130]](https://github.com/Navideck/universal_ble/pull/32/files#diff-c744a38517e9dcb9177126ffd1392b9decc2b53e55772d4270f4387a4f4c9357R1100-R1130) ```diff -if ((propertiesValue & GattCharacteristicProperties::Broadcast) != GattCharacteristicProperties::None) -{ - properties.push_back(static_cast(CharacteristicProperty::broadcast)); -} -... -if ((propertiesValue & GattCharacteristicProperties::ExtendedProperties) != GattCharacteristicProperties::None) -{ - properties.push_back(static_cast(CharacteristicProperty::extendedProperties)); +std::map propertyMap = { + {GattCharacteristicProperties::Broadcast, CharacteristicProperty::broadcast}, + ... + {GattCharacteristicProperties::ExtendedProperties, CharacteristicProperty::extendedProperties} +}; +for (const auto& [key, value] : propertyMap) { + if ((propertiesValue & key) != GattCharacteristicProperties::None) { + properties.push_back(static_cast(value)); + } } ```
Possible issue	Add error handling to the `DiscoverServicesAsync` function. ___ Ensure proper error handling for the `DiscoverServicesAsync` function, as it currently lacks any mechanism to report back errors in service discovery. [windows/src/universal_ble_plugin.cpp [1087-1089]](https://github.com/Navideck/universal_ble/pull/32/files#diff-c744a38517e9dcb9177126ffd1392b9decc2b53e55772d4270f4387a4f4c9357R1087-R1089) ```diff void UniversalBlePlugin::DiscoverServicesAsync(BluetoothDeviceAgent &bluetoothDeviceAgent, std::function reply)> result) { try { ... } + catch (const std::exception& e) + { + // Handle or log the error appropriately + result(Error(e.what())); + } ```
Performance	Use `emplace_back` for efficiency when adding elements to vectors. ___ Use `emplace_back` instead of `push_back` when adding elements to `properties` and `universalCharacteristics` to avoid unnecessary copies and improve performance. [windows/src/universal_ble_plugin.cpp [1102-1133]](https://github.com/Navideck/universal_ble/pull/32/files#diff-c744a38517e9dcb9177126ffd1392b9decc2b53e55772d4270f4387a4f4c9357R1102-R1133) ```diff -properties.push_back(static_cast(CharacteristicProperty::broadcast)); +properties.emplace_back(static_cast(CharacteristicProperty::broadcast)); ... -universalCharacteristics.push_back( +universalCharacteristics.emplace_back( flutter::CustomEncodableValue(UniversalBleCharacteristic(to_uuidstr(c.Uuid()), properties))); ```
Performance	Pass `std::function` parameters as const references. ___ Specify the `result` parameter as a const reference to avoid unnecessary copies and improve performance. [windows/src/universal_ble_plugin.h [114]](https://github.com/Navideck/universal_ble/pull/32/files#diff-5e53d816e18b3a55d63090396aac40ab0c2159f0bf24d5ae5687d003c22c809fR114-R114) ```diff -void DiscoverServicesAsync(BluetoothDeviceAgent &bluetoothDeviceAgent, std::function reply)>); +void DiscoverServicesAsync(BluetoothDeviceAgent &bluetoothDeviceAgent, const std::function reply)>& result); ```
Best practice	Use `auto` for variable type deduction. ___ Consider using `auto` for the type of `universalCharacteristics` and `properties` to make the code cleaner and more adaptable to type changes. [windows/src/universal_ble_plugin.cpp [1094-1098]](https://github.com/Navideck/universal_ble/pull/32/files#diff-c744a38517e9dcb9177126ffd1392b9decc2b53e55772d4270f4387a4f4c9357R1094-R1098) ```diff -flutter::EncodableList universalCharacteristics; -flutter::EncodableList properties = flutter::EncodableList(); +auto universalCharacteristics = flutter::EncodableList(); +auto properties = flutter::EncodableList(); ```

✨ Improve tool usage guide:

**Overview:** The `improve` tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on a PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L78) related to the improve tool (`pr_code_suggestions` section), use the following template: ``` /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_code_suggestions] some_config1=... some_config2=... ``` See the improve [usage page](https://pr-agent-docs.codium.ai/tools/improve/) for a comprehensive guide on using this tool.

Navideck / universal_ble