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

Fix doxygen for Serial and RawSerial #8413

Merged
merged 6 commits into from
Oct 19, 2018
Merged

Conversation

yennster
Copy link
Contributor

Description

This PR fixes the doxygen for Serial and RawSerial, removing protected member functions and attributes and adding clarification

Pull request type

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

@NirSonnenschein
Copy link
Contributor

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 14, 2018

Build : SUCCESS

Build number : 3354
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8413/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Oct 14, 2018

@mbed-ci
Copy link

mbed-ci commented Oct 15, 2018

drivers/Serial.h Outdated
*/
Serial(PinName tx, PinName rx, int baud);

/* Stream gives us a FileHandle with non-functional poll()/readable()/writable. Pass through
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this comment being removed? Even you want to get rid of the "should be deprecated note" - which I still think is true - the rest of the comment is meaningful, clarifying that we're specifically bypassing the Stream::readable() that we inherited here because it's broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjbracey-arm I reverted the removal as it's not related to the doxygen anyways

@@ -88,6 +88,7 @@ class RawSerial: public SerialBase, private NonCopyable<RawSerial> {

int printf(const char *format, ...);

#if !(DOXYGEN_ONLY)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protected methods are part of the API, so should really be documented.

These are overrides of the methods in SerialBase, so should be documented there - then if we have inheritance turned on, we don't need further documentation here.

Copy link
Contributor Author

@yennster yennster Oct 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjbracey-arm #8413 (comment)

@@ -63,10 +63,10 @@ class Serial : public SerialBase, public Stream, private NonCopyable<Serial> {
* @param tx Transmit pin
* @param rx Receive pin
* @param name The name of the stream associated with this serial port (optional)
* @param baud The baud rate of the serial port (optional, defaults to MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE)
* @param baud The baud rate of the serial port (optional, defaults to MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE or 9600)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so "or 9600"? I guess you mean platform.default_serial_baud_rate itself defaults to 9600?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjbracey-arm Yeah, MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE is 9600

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjbracey-arm I wanted to add the or 9600 because with just a quick glance I don't care about the MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE configuration parameter, I want to know the actual numerical value it defaults to

@@ -99,13 +95,15 @@ class Serial : public SerialBase, public Stream, private NonCopyable<Serial> {
return SerialBase::writeable();
}

#if !(DOXYGEN_ONLY)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think _mutex should have been private here, hence excluded that way? And the overridden protected functions should have been already documented in Stream and SerialBase, so picked up by INHERIT_DOCS.

If you're globally trying to exclude protected API methods (despite them being API), then does this trick work? https://stackoverflow.com/questions/11316663/is-it-possible-to-prevent-doxygen-from-outputting-protected-members/11316802

Also, as EXTRACT_ALL is off, aren't these just omitted by being undocumented anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnotherButler Can you help address the doxygen fanciness?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not omitted. Our published site shows the protected member functions.

This PR follows the method we know works. For further Doxygen questions, I suggest contacting @SenRamakri of the core OS team. The doxygen_options.json and doxyfile_options files live in mbed-os, and he typically knows a lot more about them than I do. @c1728p9 also recently put up a PR addressing Doxygen options and may also be able to help.

@yennster
Copy link
Contributor Author

@AnotherButler Can you help clarify which portions of this API I should be hiding in the doxygen and what is the correct way to do so?

@AnotherButler
Copy link
Contributor

AnotherButler commented Oct 15, 2018

As part of our quality push this quarter, we are removing all private and protected member functions (and anything else not public) from the rendered Doxygen on os.mbed.com/docs through the method implemented by @yennster

@cmonr cmonr removed the needs: CI label Oct 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants