-
Notifications
You must be signed in to change notification settings - Fork 178
Create itm.md #413
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
Create itm.md #413
Conversation
Documentation for the new ITM HAL API: ARMmbed/mbed-os#5956
@AnotherButler as we discussed. |
``` | ||
#include "SerialWireOutput.h" | ||
|
||
FileHandle* mbed::mbed_override_console(int fd) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style update
FileHandle *mbed::mbed_override_console(int fd)
{
}
Copy edit file for active voice, American English and precise language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcuschangarm Thanks for the PR. Please address my comments and queries.
@@ -0,0 +1,34 @@ | |||
### Instrumented Trace Macrocell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this API have spaces in it, or does it follow the same formatting as the others ("InstrumentedTraceMacrocell")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the question. Instrumented Trace Macrocell is the name of the module like Low Power Ticker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the ResetReason API, which is has no space and both Rs capitalized because we're referring to the class. Is it the same with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, no. This is a physical component defined in the architecture: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0314h/Chdegecg.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcuschangarm - Looks good, it would be great if you can add that ITM link in the documentation itself. Its not a blocker though, so I'll approve.
##### Undefined behavior | ||
|
||
- The debug clock frequency is left undefined because the most optimal frequency varies from target to target. It is up to each target's owner to choose a frequency that doesn't interfere with normal operation and that the owner's preferred debug monitor supports. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query: Why are there no notes, dependencies or implementations? Please complete these sections.
##### Defined behavior | ||
|
||
- Targets must implement the function `void itm_init(void)` and add `ITM` to the `device_has` section in `target.json`. | ||
- When `void itm_init(void)` is called, the debug clock for the ITM must be initialized and the SWO pin configured for debug output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query: Who or what is calling that function? Also, who or what is initializing the debug clock for the ITM and configuring the SWO pin for debug output? You, the user, or something else?
@AnotherButler I've reorganized the text and added the missing sections based on the other document you showed me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we need to add, once the doxygen is regenrated for master, is link to the API doxy.
The PR just got merged! Who can pull the doxygen lever? 😄 |
@AnotherButler can :) |
Copy edit for active voice, consistent tense and proper formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments and queries for you to address.
|
||
##### Defined behavior | ||
|
||
When initialized, writing data to the ITM stimulus registers results in the data being transmitted over the SWO line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query: When what is initialized, by whom or by what?
Query: Who or what transmits the data over the SWO line? The ITM?
#### Implementing the ITM API | ||
|
||
- You must implement the function `itm_init`. When the function is called: | ||
- The debug clock for the ITM must be initialized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query: Do you, the user initialize the debug clock for the ITM?
|
||
- You must implement the function `itm_init`. When the function is called: | ||
- The debug clock for the ITM must be initialized. | ||
- The SWO pin must be configured for debug output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query: Do you, the user, configure the SWO pin for debug output?
I think that should address the ambiguity? 😄 |
Copy edit for consistent tense and brevity.
@bulislaw Do we already have a Doxy branch for this? |
@marcuschangarm Because this has merged into master, it should be in the generated Doxy: https://os-doc-builder.test.mbed.com/docs/development/mbed-os-api-doxy/annotated.html However, I don't see it here. Could you please help me find it? Or check the mbed-os exclusions if it's not there? |
I don't see it either - but there seems to be a lot of weirdness on those auto generated pages. |
@SenRamakri Can you please help with this? |
@@ -0,0 +1,34 @@ | |||
### Instrumented Trace Macrocell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcuschangarm - Looks good, it would be great if you can add that ITM link in the documentation itself. Its not a blocker though, so I'll approve.
I need to make sure this is in the Doxy and include a link to the Doxy before merging. |
@SenRamakri Could you please help me find this in the Doxy? |
Note to self: @SenRamakri helped me find link: https://os-doc-builder.test.mbed.com/docs/development/mbed-os-api-doxy/group__itm__hal.html |
Add missing link.
Documentation for the new ITM HAL API: ARMmbed/mbed-os#5956