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

Fix discover services on Windows #32

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

rohitsangwan01
Copy link
Contributor

@rohitsangwan01 rohitsangwan01 commented Mar 28, 2024

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 codiumai-pr-agent bot added the enhancement New feature or request label Mar 28, 2024
    Copy link

    PR Description updated to latest commit (1eea381)

    Copy link

    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.

    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 by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    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]

    -if ((propertiesValue & GattCharacteristicProperties::Broadcast) != GattCharacteristicProperties::None)
    -{
    -  properties.push_back(static_cast<int>(CharacteristicProperty::broadcast));
    -}
    -...
    -if ((propertiesValue & GattCharacteristicProperties::ExtendedProperties) != GattCharacteristicProperties::None)
    -{
    -  properties.push_back(static_cast<int>(CharacteristicProperty::extendedProperties));
    +std::map<GattCharacteristicProperties, CharacteristicProperty> 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<int>(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]

     void UniversalBlePlugin::DiscoverServicesAsync(BluetoothDeviceAgent &bluetoothDeviceAgent, std::function<void(ErrorOr<flutter::EncodableList> 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]

    -properties.push_back(static_cast<int>(CharacteristicProperty::broadcast));
    +properties.emplace_back(static_cast<int>(CharacteristicProperty::broadcast));
     ...
    -universalCharacteristics.push_back(
    +universalCharacteristics.emplace_back(
         flutter::CustomEncodableValue(UniversalBleCharacteristic(to_uuidstr(c.Uuid()), properties)));
     
    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]

    -void DiscoverServicesAsync(BluetoothDeviceAgent &bluetoothDeviceAgent, std::function<void(ErrorOr<flutter::EncodableList> reply)>);
    +void DiscoverServicesAsync(BluetoothDeviceAgent &bluetoothDeviceAgent, const std::function<void(ErrorOr<flutter::EncodableList> 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]

    -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 every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @fotiDim fotiDim force-pushed the fix_discover_services_on_windows branch from 129f205 to 603b934 Compare March 28, 2024 06:46
    @rohitsangwan01 rohitsangwan01 merged commit 9e51e9e into main Mar 28, 2024
    @fotiDim fotiDim deleted the fix_discover_services_on_windows branch April 26, 2024 06:43
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants