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

Kernel.h doxygen update #8435

Merged
merged 3 commits into from Nov 7, 2018

Conversation

Projects
None yet
5 participants
@SenRamakri
Contributor

SenRamakri commented Oct 15, 2018

Description

Minor fix for Kernel.h doxygen comments

Pull request type

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

@SenRamakri SenRamakri requested a review from kjbracey-arm Oct 15, 2018

*/
void attach_idle_hook(void (*fptr)(void));
/** Attach a function to be called when a task is killed
/** Attach a function to be called when a task is terminated

This comment has been minimized.

@SenRamakri

SenRamakri Oct 15, 2018

Contributor

"killed" sounds like a forceful action, but in reality the callback gets called when a thread terminates. So changing the wording to "terminated"

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 16, 2018

Contributor

Good point, but I think the wording should actually be what you said in the comment: "when a thread terminates".

@param fptr pointer to the function to be called
@note You may call this function from ISR context.
@note You can call this function from ISR context.

This comment has been minimized.

@SenRamakri

SenRamakri Oct 15, 2018

Contributor

@kjbracey-arm - I think we need to be stricter, so Im changing to "can", please review.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 16, 2018

Contributor

The "may" wording is used in lots of other files, so if you want to change it should be consistent. Although it's "cannot" in the same places. I'd say the docs team probably have a global style rule here, so check with them.

I think "may" is clearer for the positive case, personally - it says you're "permitted" to, rather than just being "able" to.

This comment has been minimized.

@SenRamakri

SenRamakri Oct 17, 2018

Contributor

The question is for a new user wanting to use this function doesn't get much info from reading this. When we say "may" it sounds like user should go find if its even possible for the specific context he wants to use. I also wonder why someone would want to use these functions from ISR context.

This comment has been minimized.

@cmonr

cmonr Oct 19, 2018

Contributor

@kjbracey-arm ^^^
(It wasn't to clear to me that he had already replied)

This comment has been minimized.

@SenRamakri

SenRamakri Oct 22, 2018

Contributor

@kjbracey-arm - Can you please look at my comment above? You did raise a good point about consistency but I do think that can be addressed in other parts while we can document this better still.

@cmonr cmonr added the needs: review label Oct 15, 2018

*/
void attach_idle_hook(void (*fptr)(void));
/** Attach a function to be called when a task is killed
/** Attach a function to be called when a task is terminated

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 16, 2018

Contributor

Good point, but I think the wording should actually be what you said in the comment: "when a thread terminates".

@param fptr pointer to the function to be called
@note You may call this function from ISR context.
@note You can call this function from ISR context.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Oct 16, 2018

Contributor

The "may" wording is used in lots of other files, so if you want to change it should be consistent. Although it's "cannot" in the same places. I'd say the docs team probably have a global style rule here, so check with them.

I think "may" is clearer for the positive case, personally - it says you're "permitted" to, rather than just being "able" to.

@cmonr cmonr added needs: work and removed needs: review labels Oct 18, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 18, 2018

@SenRamakri Please review @kjbracey-arm's comments

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Oct 19, 2018

@cmonr - I have already responded to @kjbracey-arm comments. I'm waiting for his thoughts on that.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 29, 2018

@kjbracey-arm Mind responing to @SenRamakri? Thoughts on how to move this forward?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 29, 2018

I'm unsure what the current "state-of-the-art" of "interrupt safety" documentation is. Is there a current equivalent of this page? https://docs.mbed.com/docs/mbed-os-handbook/en/5.2/concepts/thread_safety/ ? I see some files still use those terms.

So I'm not even sure if we should be tinkering with this wording (which a lot of files use) or using something totally different.

If we are tinkering this this wording, "may" seems more formally correct to me than "can", so I'm not particularly keen on the change. Obviously you /can/ - no-one can stop you - but are you permitted to?

I think this is one for @AnotherButler, really, but...

https://dictionary.cambridge.org/grammar/british-grammar/can-could-or-may
https://en.oxforddictionaries.com/usage/can-or-may

And yes, no idea why anyone would want to call those from ISR - I expect the docs are the result of someone inspecting what looked like it would work and annotating.

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Oct 29, 2018

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_KernelDoxyUpdate branch from 8d9ef11 to 4cdcdc1 Oct 29, 2018

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Oct 29, 2018

@kjbracey-arm @AnotherButler - I have pushed new changes with review comments addressed. I also have reverted from "can" to "may" as I think it doesn't matter much as mentioned in the descriptions in the links provided by @kjbracey-arm, so please review again.

Edit Kernel.h
Make minor copy edits for active voice, branding and deletion of extra spaces.

@0xc0170 0xc0170 added needs: review and removed needs: work labels Oct 31, 2018

Please rereview

@0xc0170

0xc0170 approved these changes Nov 6, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Nov 7, 2018

Note: This PR is now a part of a rollup PR (#8663).

No further work is needed here, as once that PR is merged, this PR will also be closed and marked as merged.

If any more commits are made in this PR, this PR will remain open and have to go through CI on its own.

@0xc0170 0xc0170 merged commit 2418d9c into ARMmbed:master Nov 7, 2018

11 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/cloud-client-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 546 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10164 cycles (+903 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: CI label Nov 7, 2018

@cmonr cmonr removed the rollup PR label Nov 7, 2018

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