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

Asynchronous Serial API fixes and refactoring #9476

Merged
merged 3 commits into from Mar 28, 2019

Conversation

Projects
None yet
10 participants
@lrusinowicz
Copy link
Contributor

commented Jan 23, 2019

Description

  1. As RX and TX flows are separate on Serial device, read and write
    functionalities should be completely separate, including any deep
    sleep locking etc.
  2. Aborting of asynchronous operation is necessarily hazardous, as
    operation can end in interrupt anywhere from the point of decision
    until it is actually aborted down the abort_...() method.
    Proper deep sleep unlocking requires then use of the critical
    section and should be contained completely within API implementation.
  3. Because of point 2, API user should be able to call abort_..()
    operation independently of whether the operation is pending or not.
  4. User may want to use asynchronous API without a callback (especially
    for write operations), for example in a command-response scheme
    end of write operation is usually meaningless. The intuitive
    method is to submit NULL pointer for a callback. For this reason
    depending on the _callback field in determining whether the
    operation is in progress seems to be uncertain.
  5. Updated API documentation to cover above issues and to explicitly
    point out that any event ends the operation.

Pull request type

[x] Fix
[x] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@0xc0170

@ciarmcom ciarmcom requested a review from 0xc0170 Jan 23, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

@lrusinowicz, thank you for your changes.
@0xc0170 @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jan 23, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

@ARMmbed/mbed-os-hal @kjbracey-arm Please take a look at this since this PR modifies Serial behavior

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

Afraid I don't feel terribly qualified to comment - I've never dared use the asynchronous serial API. Not sure who to suggest to cover it.

From what I can see though, this does look like a solid improvement.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

@kjbracey-arm Thanks for the input.

Wondering if @ARMmbed/mbed-os-coresw could take a look at this as well?

@loverdeg-ep

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Afraid I don't feel terribly qualified to comment - I've never dared use the asynchronous serial API.

@kjbracey-arm Any particular reason why? I default to async api's in most cases, however I've all but confirmed that the async api's in Mbed OS are a 2nd priority.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

It would be easier to review if the commit is split to few logical parts.

I can see from review:

  • adding set flags for tx/rx
  • proper locking addition
  • updating docs

Changes are explained in the commit msg but it's hard to follow all in one.

lrusinowicz added some commits Feb 12, 2019

Asynchronous Serial API fixes and refactoring, part 1
1. As RX and TX flows are separate on Serial device, read and write
   functionalities should be completely separate, including any deep
   sleep locking etc.
2. User may want to use asynchronous API without a callback (especially
   for write operations), for example in a command-response scheme
   end of write operation is usually meaningless. The intuitive
   method is to submit NULL pointer for a callback. For this reason
   depending on the _callback field in determining whether the
   operation is in progress seems to be uncertain, so introduced
   additional flags for this purpose.
Asynchronous Serial API fixes and refactoring, part 2
Aborting of asynchronous operation is necessarily hazardous, as
operation can end in interrupt anywhere from the point of decision
until it is actually aborted down the abort_...() method.
Proper deep sleep unlocking requires then use of the critical
section and should be contained completely within API implementation.
Asynchronous Serial API fixes and refactoring, part 3
Updated API documentation to better explain API operation and to explicitly
point out that any event ends the operation.

@lrusinowicz lrusinowicz force-pushed the lrusinowicz:asynch_serial_api branch to 8586528 Feb 12, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

@ARMmbed/mbed-os-hal Any chance someone would be able to look at this sometime soon?

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@donatieng

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Looks like good improvements from my end but this is a driver change, so very much @ARMmbed/mbed-os-core's territory 😉.

@SenRamakri @deepikabhavnani what do you think?

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@SenRamakri @deepikabhavnani what do you think?

Async driver changes look fine to me, it will be good to have a review from @c1728p9 for deep sleep lock and unlock cases

@c1728p9

c1728p9 approved these changes Mar 5, 2019

@cmonr

cmonr approved these changes Mar 27, 2019

Copy link
Contributor

left a comment

LGTM

@cmonr cmonr added needs: CI and removed needs: review labels Mar 27, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 28, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Mar 28, 2019

cmonr added a commit to cmonr/mbed-os that referenced this pull request Mar 28, 2019

Merge pull request ARMmbed#9476 from lrusinowicz/asynch_serial_api
Asynchronous Serial API fixes and refactoring

@cmonr cmonr merged commit d3db0a8 into ARMmbed:master Mar 28, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9156 cycles (-73 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.