Kernel.h doxygen update #8435

SenRamakri · 2018-10-15T19:01:39Z

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 · 2018-10-15T19:03:52Z

rtos/Kernel.h

 */
 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


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

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

SenRamakri · 2018-10-15T19:03:55Z

rtos/Kernel.h

  @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.


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

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.

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.

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

@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.

kjbracey · 2018-10-16T13:06:34Z

rtos/Kernel.h

 */
 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


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

kjbracey · 2018-10-16T13:08:26Z

rtos/Kernel.h

  @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.


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 · 2018-10-18T22:27:52Z

@SenRamakri Please review @kjbracey-arm's comments

SenRamakri · 2018-10-19T15:47:29Z

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

cmonr · 2018-10-29T14:01:42Z

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

kjbracey · 2018-10-29T14:31:14Z

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 · 2018-10-29T14:34:08Z

The equivalent of that page is https://os.mbed.com/docs/latest/porting/thread-safety-and-porting.html

SenRamakri · 2018-10-29T18:56:45Z

@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.

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

Please rereview

cmonr · 2018-11-07T03:33:26Z

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.

SenRamakri requested a review from kjbracey October 15, 2018 19:01

SenRamakri commented Oct 15, 2018

View reviewed changes

cmonr added the needs: review label Oct 15, 2018

kjbracey previously requested changes Oct 16, 2018

View reviewed changes

kjbracey mentioned this pull request Oct 17, 2018

Kernel documentation updated with new additions to Kernel namespace ARMmbed/mbed-os-5-docs#792

Merged

0xc0170 requested a review from AnotherButler October 17, 2018 23:30

AnotherButler approved these changes Oct 18, 2018

View reviewed changes

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

SenRamakri added 2 commits October 29, 2018 13:47

Kernel.h doxygen update

ad88218

Updated attach_thread_terminate_hook doxygen comments

4cdcdc1

SenRamakri force-pushed the sen_KernelDoxyUpdate branch from 8d9ef11 to 4cdcdc1 Compare October 29, 2018 18:54

Edit Kernel.h

2418d9c

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

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

0xc0170 approved these changes Nov 6, 2018

View reviewed changes

kjbracey approved these changes Nov 6, 2018

View reviewed changes

cmonr added needs: CI release-version: 5.10.4 rollup PR and removed needs: review labels Nov 6, 2018

cmonr mentioned this pull request Nov 7, 2018

Rollup PR: Docs + small fixes #8663

Merged

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

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

cmonr removed the rollup PR label Nov 7, 2018

Kernel.h doxygen update #8435

Kernel.h doxygen update #8435

Uh oh!

Conversation

SenRamakri commented Oct 15, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Oct 18, 2018

Uh oh!

SenRamakri commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Oct 29, 2018

Uh oh!

kjbracey commented Oct 29, 2018

Uh oh!

AnotherButler commented Oct 29, 2018

Uh oh!

SenRamakri commented Oct 29, 2018

Uh oh!

cmonr commented Nov 7, 2018

Uh oh!

Uh oh!

SenRamakri commented Oct 19, 2018 •

edited

Loading