Skip to content

Jira 795, read() is blocking selectable, git 383 #418

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 23, 2017
Merged

Jira 795, read() is blocking selectable, git 383 #418

merged 1 commit into from
Feb 23, 2017

Conversation

SidLeung
Copy link
Contributor

Mods:

  1. Add an input parameter to BLECharacteristic::read() for blocking
    call selection.
  2. By default, read() is a blocking call.

File changes:

  1. libraries/CurieBLE/src/BLECharacteristic.cpp:
    • Method read() added blocking selection.
  2. libraries/CurieBLE/src/BLECharacteristic.h:
    • Prototype for read(), default is blocking.
  3. libraries/CurieBLE/src/internal/BLECallbacks.cpp:
    • Added parameter checking in profile_read_rsp_process().
  4. libraries/CurieBLE/src/internal/BLECharacteristicImp.cpp
    • The implementation of read() added waiting for resp if
      blocking is selected.
  5. libraries/CurieBLE/src/internal/BLECharacteristicImp.h:
    • prototyping.

@SidLeung
Copy link
Contributor Author

@bigdinotech @eriknyquist , please review this code change. @noelpaz @russmcinnis , please verify this fix. Please use the example sketch, peripheral_explorer, as a guideline for testing the resolution.

@SidLeung SidLeung added this to the Deneb milestone Jan 31, 2017
@SidLeung SidLeung mentioned this pull request Feb 1, 2017
Closed
eriknyquist
eriknyquist reviewed Feb 1, 2017
View reviewed changes
libraries/CurieBLE/src/internal/BLECharacteristicImp.h
*/
bool read();
Copy link
Contributor

Choose a reason for hiding this comment

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

So this doesn't change any existing code, right? i.e. if I call read() with no arguments, it will block by default, yes?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct.

eriknyquist
eriknyquist reviewed Feb 1, 2017
View reviewed changes
libraries/CurieBLE/src/internal/BLECharacteristicImp.h
@@ -324,7 +324,7 @@ class BLECharacteristicImp: public BLEAttribute{
bt_gatt_subscribe_params_t _sub_params;
bool _subscribed;

bool _reading;
volatile bool _reading;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be volatile?

Copy link
Contributor

@sgbihu sgbihu Feb 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please note last two changes in BLECharacteristicImp.cpp. It need to make compiler not optimize the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we typing it as volatile so that it compile? Is this change temporary then? What is the penalty as far as code size?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so why do you need to force optimization off? Who is accessing this variable out of normal execution flow? Describe the situation that will occur if _reading is not volatile.

Copy link
Contributor

Choose a reason for hiding this comment

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

When read response come back, the setValue will be called and clear the reading flag. Because the response is asynchronous. So the below code may optimized for the value was set true before call while.

_reading = true;

while(_reading)
{
......
}

@SidLeung
Copy link
Contributor Author

SidLeung commented Feb 3, 2017

@noelpaz , @russmcinnis , please provide feed back on this PR.

eriknyquist
eriknyquist approved these changes Feb 8, 2017
View reviewed changes
Copy link
Contributor

@eriknyquist eriknyquist left a comment

Choose a reason for hiding this comment

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

Talked with Sidney, who gave me a good explanation of the requirement for volatile use.

noelpaz
noelpaz reviewed Feb 8, 2017
View reviewed changes
Copy link
Contributor

@noelpaz noelpaz left a comment

Choose a reason for hiding this comment

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

I am starting BAT Test on this and then will discuss with @SidLeung for focused unit testing and will use analyzer if needed.

libraries/CurieBLE/src/internal/BLECharacteristicImp.h
@@ -324,7 +324,7 @@ class BLECharacteristicImp: public BLEAttribute{
bt_gatt_subscribe_params_t _sub_params;
bool _subscribed;

bool _reading;
volatile bool _reading;
Copy link
Contributor

Choose a reason for hiding this comment

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

So we typing it as volatile so that it compile? Is this change temporary then? What is the penalty as far as code size?

@noelpaz noelpaz requested a review from russmcinnis February 9, 2017 00:14
@noelpaz
Copy link
Contributor

noelpaz commented Feb 9, 2017

@russmcinnis is testing this functionality. But we cannot do an end to end testing unless we resolve the conflicts. Pinging @SidLeung

@SidLeung
Copy link
Contributor Author

SidLeung commented Feb 9, 2017

@noelpaz , @russmcinnis , conflict resolved, please proceed with system testing. Thanks.

@SidLeung SidLeung assigned SidLeung and unassigned russmcinnis and noelpaz Feb 10, 2017
@SidLeung
Copy link
Contributor Author

@noelpaz @russmcinnis , please retest this PR. It has resolved the hanging issue due to disconnection in the middle of reading.

@bigdinotech @eriknyquist , please review the additional code change for disconnection detection.

@sgbihu @SidLeung
Jira 795, read() is blocking selectable, git 383
bc56500 
Mods:
1. Add an input parameter to BLECharacteristic::read() for blocking
   call selection.
2. By default, read() is a blocking call.

File changes:
1. libraries/CurieBLE/src/BLECharacteristic.cpp:
   - Method read() added blocking selection.
2. libraries/CurieBLE/src/BLECharacteristic.h:
   - Prototype for read(), default is blocking.
3. libraries/CurieBLE/src/internal/BLECallbacks.cpp:
   - Added parameter checking in profile_read_rsp_process().
4. libraries/CurieBLE/src/internal/BLECharacteristicImp.cpp
   - The implementation of read() added waiting for resp if
     blocking is selected.
5. libraries/CurieBLE/src/internal/BLECharacteristicImp.h:
   - prototyping.
@SidLeung
Copy link
Contributor Author

@yashaswini-hanji , please merge the PR to main and close out Jira 795.

@ndgbuilder ndgbuilder merged commit bc56500 into arduino:master Feb 23, 2017
@yashaswini-hanji
Copy link
Contributor

PR merged and Jira 795 is closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noelpaz noelpaz noelpaz left review comments

@sgbihu sgbihu sgbihu left review comments

@eriknyquist eriknyquist eriknyquist approved these changes

@bigdinotech bigdinotech Awaiting requested review from bigdinotech

@russmcinnis russmcinnis Awaiting requested review from russmcinnis

Assignees

@SidLeung SidLeung

Labels
Under Review
Projects
None yet
Milestone
Deneb
Development

Successfully merging this pull request may close these issues.
7 participants
@SidLeung @noelpaz @yashaswini-hanji @eriknyquist @sgbihu @russmcinnis @ndgbuilder