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

Doxygen comments fixes #5372

Merged
merged 3 commits into from Nov 9, 2017

Conversation

Projects
None yet
6 participants
@SenRamakri
Contributor

SenRamakri commented Oct 24, 2017

This is the change for updating/fixing Doxygen comments/config to better organize the doxygen docs.

@SenRamakri SenRamakri requested review from sg- and AnotherButler Oct 24, 2017

drivers/UARTSerial.h Outdated
@@ -39,6 +39,13 @@
namespace mbed {
/** \addtogroup drivers */
/** A serial port implementation

This comment has been minimized.

@sg-

sg- Oct 25, 2017

Member

This could use some work. It sounds so boring.

rtos/rtos_idle.h Outdated
*/
/**
@note
Sets the hook function called by by idle task.

This comment has been minimized.

@0xc0170

0xc0170 Oct 25, 2017

Member

by just once

rtos/rtos_idle.h Outdated
/**
@note
Sets the hook function called by by idle task.
@param fptr Hook function pointer.

This comment has been minimized.

@0xc0170

0xc0170 Oct 25, 2017

Member

. should not be there?

platform/mbed_assert.h Outdated
@@ -39,6 +43,19 @@ void mbed_assert_internal(const char *expr, const char *file, int line);
}
#endif
/** MBED_ASSERT
* Declare runtime assertions, results in runtime error if condition is false

This comment has been minimized.

@AnotherButler

AnotherButler Oct 25, 2017

Contributor

This is a nitpick, but please change the comma to a semicolon

@sg-

I dont understand all the changes to groups and why they're needed. the commit history needs to be rewritten or all part of one commit.

@@ -32,7 +32,7 @@ DOXYFILE_ENCODING = UTF-8
# title of most generated pages and in a few other places.
# The default value is: My Project.
PROJECT_NAME = "My Project"
PROJECT_NAME = "Mbed OS Reference"

This comment has been minimized.

@sg-

sg- Oct 25, 2017

Member

This change is also needed in the json version

This comment has been minimized.

@SenRamakri

SenRamakri Oct 26, 2017

Contributor

Can you please point me to which json file I should add this change? Then the group changes are only to make navigation through the API docs easier. It doesn't change or move any APIs or the groups. See the attached image. It shows how the grouping/API looks like with these changes. Earlier everything was showing up in the index.html page itself and this change cleans up lot of those. Its based on our discussion to align things like how HAL group was organized.
api_groups

This comment has been minimized.

@sg-

sg- Oct 28, 2017

Member

doxyfile_options.json

This comment has been minimized.

@SenRamakri

SenRamakri Oct 31, 2017

Contributor

@sg- The contents of json file is as below -
{
"ENABLE_PREPROCESSING": "YES",
"MACRO_EXPANSION": "YES",
"EXPAND_ONLY_PREDEF": "NO",
"SEARCH_INCLUDES": "YES",
"INCLUDE_PATH": "",
"INCLUDE_FILE_PATTERNS": "",
"PREDEFINED": "DOXYGEN_ONLY DEVICE_ANALOGIN DEVICE_ANALOGOUT DEVICE_CAN DEVICE_ETHERNET DEVICE_EMAC DEVICE_FLASH DEVICE_I2C DEVICE_I2CSLAVE DEVICE_I2C_ASYNCH DEVICE_INTERRUPTIN DEVICE_LOWPOWERTIMER DEVICE_PORTIN DEVICE_PORTINOUT DEVICE_PORTOUT DEVICE_PWMOUT DEVICE_RTC DEVICE_TRNG DEVICE_SERIAL DEVICE_SERIAL_ASYNCH DEVICE_SERIAL_FC DEVICE_SLEEP DEVICE_SPI DEVICE_SPI_ASYNCH DEVICE_SPISLAVE DEVICE_STORAGE "MBED_DEPRECATED_SINCE(f, g)=" "MBED_ENABLE_IF_CALLBACK_COMPATIBLE(F, M)="",
"EXPAND_AS_DEFINED": "",
"SKIP_FUNCTION_MACROS": "NO",
"EXCLUDE_PATTERNS": "/tools/ /targets/ /FEATURE_/* /features/mbedtls/ /features/storage/ /features/unsupported/ /features/filesystem/ /BUILD/ /rtos/TARGET_CORTEX/rtx/* /cmsis/ /features/FEATURES_"
}
Looks like not changes are needed in that, can you please confirm?

This comment has been minimized.

@sg-

sg- Oct 31, 2017

Member

If you dont add that here, it will not be present in the online hosted version. All changes to the local doxyfile need to be accounted for in the json version. Otherwise it is assumed that the tool defaults will be used.

This comment has been minimized.

@SenRamakri

SenRamakri Oct 31, 2017

Contributor

Ah yes, I see your point now, updated json as well now. Please review.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 26, 2017

👍 for adding the groups. You could have added a screenshot how it looked prior this change and with this change. To get a picture of how this patch organizes the doc for our API.

As pinpointed above, can you squash these changes into one (I believe all of them can be done within one commit - same logical change for all the files).

I noticed also some are called functions and some class but those functions like CThunk, it is actually a class, can you also check those?

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_PlatformDoxygenUpdates branch to 3ad2984 Oct 26, 2017

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Oct 26, 2017

Review comments fixed with squashed commit.

@0xc0170

One question regarding debug builds, if that note is correct. The rest LGTM

* Declare runtime assertions: results in runtime error if condition is false
*
* @note
* Use of MBED_ASSERT is limited debug builds only.

This comment has been minimized.

@0xc0170

0xc0170 Oct 27, 2017

Member

Same question here as in the docs, debug builds are ?

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Oct 31, 2017

@sg- I have fixed all the review comments. can you please look into this when you get a chance?

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Nov 2, 2017

@sg- Can you please review if there is anything pending on this PR from my side?

@sg-

sg- approved these changes Nov 2, 2017

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Nov 3, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 3, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 3, 2017

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Nov 6, 2017

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit fbd9e7e into ARMmbed:master Nov 9, 2017

5 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@0xc0170 0xc0170 removed the ready for merge label Nov 9, 2017

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