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

Add units to timeout argument in EventFlags #9670

Merged
merged 1 commit into from Feb 21, 2019

Conversation

Projects
None yet
7 participants
@kegilbert
Copy link
Contributor

kegilbert commented Feb 11, 2019

Description

Add units to the timeout argument in wait_all/any:

#8894

Pull request type

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

Reviewers

@sarahmarshy
@AnotherButler

@cmonr

cmonr approved these changes Feb 11, 2019

@cmonr cmonr added the needs: review label Feb 11, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Feb 12, 2019

This phrasing occurs in quite a lot of other RTOS header files, as they were a copy-and-paste from CMSIS-RTOS docs (but might be CMSIS-RTOS v1 rather than v2?). Would be worth updating the others if doing EventFlags, as I think consistency is important here - they all have exactly the same time concept.

I can immediately think of Thread, ThisThread, Semaphore, Mutex, possibly ConditionVariable.

As a sort of compromise, some of those seem to have kept the phrasing, but renamed the timeout variable to millisec.

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

kegilbert commented Feb 12, 2019

@kjbracey-arm It looks like everything else in rtos uses the millisecond parameter name with the old description. I'm not as much of a fan of this since I find it's a bit strange to name the variable after the units and not what it does but as everything uses that I could instead change the EventFlag parameter to millisecond for consistency.

@loverdeg-ep

This comment has been minimized.

Copy link
Contributor

loverdeg-ep commented Feb 12, 2019

#9077

Edit: Lightly related issue

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 13, 2019

@loverdeg-ep Mind elaborating on your single-link comment?

@loverdeg-ep

This comment has been minimized.

Copy link
Contributor

loverdeg-ep commented Feb 13, 2019

@loverdeg-ep Mind elaborating on your single-link comment?

Kind of a personal note I suppose. Like to be able to retrace interesting & related conversations for my future self.

I'll cut it out if causing trouble.

@kjbracey-arm
Copy link
Contributor

kjbracey-arm left a comment

As per comment above, I'm going to request that this be made consistent across the rtos classes.

I don't mind whether you change this to match the others, or change all the others to match your new form here - others can express a preference.

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 13, 2019

I'll cut it out if causing trouble.

Nah, no need. That's fine.

@cmonr cmonr added needs: work and removed needs: review labels Feb 13, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 20, 2019

@kegilbert Any progress?

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

kegilbert commented Feb 20, 2019

Yup, updating this today.

Change EventFlag timeout paramter
Matches rest of RTOS class timeout parameters by using the unit name. Remove ambigious statement in reference to 0 ms being no-timeout as a timeout of 0 causes the function to not block and return immediately (osWaitForever is used as no timeout as it will wait forever)

@kegilbert kegilbert force-pushed the kegilbert:timeout_units branch to f1abca3 Feb 20, 2019

@kegilbert

This comment has been minimized.

Copy link
Contributor Author

kegilbert commented Feb 20, 2019

I additionally removed the or 0 in case of no timeout segments from the comments as that implied the opposite of what a timeout of 0 actually did to me (no timeout to me sounds like the function will never timeout which is what osWaitForever will do).

@param millisec Timeout value or 0 in case of no timeout (default: osWaitForever).

As we had in Mail.h in particular threw me off. Feel free to let me know if you disagree, I can change that back if need be.

@0xc0170 0xc0170 added the needs: CI label Feb 21, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 21, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 21, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Feb 21, 2019

@0xc0170 0xc0170 merged commit baed84a into ARMmbed:master Feb 21, 2019

27 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-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR 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-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR 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 9296 cycles (+172 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
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.