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 #559, Resolve doxygen warnings #648

Merged

Conversation

lbleier-GSFC
Copy link

@lbleier-GSFC lbleier-GSFC commented Apr 28, 2020

Describe the contribution
Fixes #559, resolving all doxygen warnings

Testing performed
Steps taken to test the contribution:

  1. Fixed warnings as listed in warnings.log
  2. Built documentation using make doc
  3. Observed relevant documentation working correctly

Expected behavior changes
A clear and concise description of how this contribution will change behavior and level of impact.

  • Documentation links and references will now work properly

Contributor Info - All information REQUIRED for consideration of pull request
Full name and company/organization/center of all contributors ("Personal" if individual work)

  • Leor Bleier NASA GSFC\Code 582

fsw/cfe-core/src/es/cfe_es_cds.h Show resolved Hide resolved
@@ -562,7 +560,7 @@ int32 CFE_SB_UnsubscribeLocal(CFE_SB_MsgId_t MsgId, CFE_SB_PipeId_t PipeId);
**
** \param[in] MsgPtr A pointer to the message to be sent. This must point
** to the first byte of the software bus message header
** (#CFE_SB_Msg_t).
** ("::"CFE_SB_Msg_t).
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why old reference didn't work? What does "::" do?

Copy link
Author

@lbleier-GSFC lbleier-GSFC Apr 28, 2020

Choose a reason for hiding this comment

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

Part of the autolink system: http:https://www.doxygen.nl/manual/autolink.html (item 3 under "Links to functions"

Copy link
Author

Choose a reason for hiding this comment

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

I double checked, I'm noticing that the literal "::" appears as well. I missed that, I may need to revisit

Copy link
Contributor

Choose a reason for hiding this comment

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

Did the # cause an error/warning? Note 3 from that reference seems to say # should work? "For Javadoc compatibility a # may be used instead of a :: in the patterns above."

Copy link
Author

Choose a reason for hiding this comment

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

It was, yes, which is why I changed it, and got no error as a result.

@@ -595,7 +593,7 @@ int32 CFE_SB_SendMsg(CFE_SB_Msg_t *MsgPtr);
**
** \param[in] MsgPtr A pointer to the message to be sent. This must point
** to the first byte of the software bus message header
** (#CFE_SB_Msg_t).
** ("::"CFE_SB_Msg_t).
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

@@ -54,7 +54,7 @@
** \cfecmdmnemonic \SB_NOOP
**
** \par Command Structure
** #CFE_SB_CmdHdr_t
** cfe_sb.h::CFE_SB_CmdHdr_t
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these show up as links? Curious why old reference didn't work

@@ -413,18 +413,16 @@ int32 CFE_SB_RemoveDest(CFE_SB_RouteEntry_t *RouteEntry, CFE_SB_DestinationD_t *
**
** \par Assumptions, External Events, and Notes:
** - For statically defined messages, a function call will not work. The
** macros #CFE_SB_CMD_HDR_SIZE and #CFE_SB_TLM_HDR_SIZE are available for use
** macros cfe_sb.h::CFE_SB_CMD_HDR_SIZE and cfe_sb.h::CFE_SB_TLM_HDR_SIZE are available for use
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these link correctly?

Copy link
Author

Choose a reason for hiding this comment

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

These are not working, I missed them too. Will fix

@@ -2,12 +2,7 @@
# Project related configuration options, shared for all cFE doxygen outputs
#---------------------------------------------------------------------------
@INCLUDE_PATH = @MISSION_SOURCE_DIR@
DOXYFILE_ENCODING = UTF-8
Copy link
Contributor

Choose a reason for hiding this comment

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

Were all deleted settings the defaults?

Copy link
Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Where can a user go find what the defaults are?

Copy link
Contributor

Choose a reason for hiding this comment

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

doxygen will create a config file with the defaults, doxygen -g configfile

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worthwhile to add a comment that says "to see defaults run doxygen -g configfile" ?

As we experienced last week, doxygen knowledge is sparse, and this might help.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's on the man page...

@lbleier-GSFC
Copy link
Author

I guess I missed a few of these. I thought they were working. I will need to revisit and push a fix

@skliper skliper changed the title Fix559 fixdocwarnings Fix #559, Resolve doxygen warnings Apr 28, 2020
@astrogeco
Copy link
Contributor

@lbleier-GSFC I like the simplification. Can you post a built version of this so we can see how these changes will affect the generated doc?

@astrogeco
Copy link
Contributor

When it's ready go ahead and tag with the "ready for CCB" label

@lbleier-GSFC
Copy link
Author

@lbleier-GSFC I like the simplification. Can you post a built version of this so we can see how these changes will affect the generated doc?

I am not quite sure what you're asking for. The entire documentation? Where would I post it?

@skliper
Copy link
Contributor

skliper commented May 5, 2020

@lbleier-GSFC I like the simplification. Can you post a built version of this so we can see how these changes will affect the generated doc?

I am not quite sure what you're asking for. The entire documentation? Where would I post it?

Likely just the pdf that gets built from the latex directory.

@skliper
Copy link
Contributor

skliper commented May 5, 2020

Drag and drop to add it to a comment here

@lbleier-GSFC
Copy link
Author

@lbleier-GSFC I like the simplification. Can you post a built version of this so we can see how these changes will affect the generated doc?

I am not quite sure what you're asking for. The entire documentation? Where would I post it?

Likely just the pdf that gets built from the latex directory.

Latex doesn't get generated, as per the config file. I don't see any relevant PDFs

@skliper
Copy link
Contributor

skliper commented May 5, 2020

@lbleier-GSFC I like the simplification. Can you post a built version of this so we can see how these changes will affect the generated doc?

I am not quite sure what you're asking for. The entire documentation? Where would I post it?

Likely just the pdf that gets built from the latex directory.

Latex doesn't get generated, as per the config file. I don't see any relevant PDFs

I wonder why we don't for doc, we do for osalguide and usersguide. Want to try enabling it and see if you can build the pdf? Enable latex, enter the build/doc/detaildesign/latex directory and type make, if it works it'll create a refman.pdf (or whatever name it's set to) which can be dropped here.

@lbleier-GSFC
Copy link
Author

@lbleier-GSFC I like the simplification. Can you post a built version of this so we can see how these changes will affect the generated doc?

I am not quite sure what you're asking for. The entire documentation? Where would I post it?

Likely just the pdf that gets built from the latex directory.

Latex doesn't get generated, as per the config file. I don't see any relevant PDFs

I wonder why we don't for doc, we do for osalguide and usersguide. Want to try enabling it and see if you can build the pdf? Enable latex, enter the build/doc/detaildesign/latex directory and type make, if it works it'll create a refman.pdf (or whatever name it's set to) which can be dropped here.

I am trying to build but it's failing, I'll need to investigate

@lbleier-GSFC
Copy link
Author

lbleier-GSFC commented May 6, 2020

@lbleier-GSFC I like the simplification. Can you post a built version of this so we can see how these changes will affect the generated doc?

I managed to build a PDF despite a ton of warnings and make having an error. I am seeing some issues (e.g. all the references for CFE_SB_SubscribeFull() on page 1444), but I can't seem to find this in the source

File:
refman.pdf

@lbleier-GSFC lbleier-GSFC added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label May 6, 2020
@astrogeco
Copy link
Contributor

CCB-20200506 - APPROVED, Open new tickets for LaTeX mission docs and missing function documentation.

@astrogeco astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label May 6, 2020
@skliper skliper added this to the 6.8.0 milestone May 7, 2020
@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB IC-20200429 labels May 8, 2020
@astrogeco
Copy link
Contributor

CCB-20200506 - APPROVED, Open new tickets for LaTeX mission docs and missing function documentation.

@skliper @lbleier-GSFC did we open tickets for these?

@lbleier-GSFC
Copy link
Author

CCB-20200506 - APPROVED, Open new tickets for LaTeX mission docs and missing function documentation.

@skliper @lbleier-GSFC did we open tickets for these?

Yes, #682 and #681, looks like the latter has already been fixed in #686

@astrogeco
Copy link
Contributor

@lbleier-GSFC can you resolve these conflicts?

@lbleier-GSFC
Copy link
Author

lbleier-GSFC commented May 8, 2020

@lbleier-GSFC can you resolve these conflicts?

I'm not quite sure what the conflict is. Both the cfeapi and cfeglossary sections of main.dox were completely removed and put into their own separate files. I will delete those sections in main.dox

@skliper
Copy link
Contributor

skliper commented May 8, 2020

I recommend rebasing your changes on top of IC, and resolve conflicts there.

@skliper
Copy link
Contributor

skliper commented May 8, 2020

Also needs squash

docs/src/cfe_api.dox Outdated Show resolved Hide resolved
@skliper
Copy link
Contributor

skliper commented May 8, 2020

@astrogeco - Resolved and squashed.

@skliper skliper removed the conflicts label May 8, 2020
@astrogeco astrogeco merged commit e79d2a7 into nasa:integration-candidate May 8, 2020
skliper added a commit to skliper/cFE that referenced this pull request May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants