Skip to content

Fix #14, Add guide on creating table files#15

Merged
jphickey merged 1 commit intonasa:masterfrom
avan989:creatingTableGuide
Oct 23, 2019
Merged

Fix #14, Add guide on creating table files#15
jphickey merged 1 commit intonasa:masterfrom
avan989:creatingTableGuide

Conversation

@avan989
Copy link
Contributor

@avan989 avan989 commented Sep 9, 2019

Describe the contribution
Fix #14, Add guide on creating table file using elf2cfetbl

Testing performed
Steps taken to test the contribution:

  1. Create a table file using elf2cfetbl

Expected behavior changes
No impact to behavior

System(s) tested on:

  • Hardware
  • OS: Ubuntu 18.04.03
  • elf2cfetbl version 3.0a

Additional context
None

Contributor Info
Anh Van, NASA Goddard

Community contributors
You must attach a signed CLA (required for acceptance) or reference one already submitted

@skliper skliper requested a review from jphickey October 1, 2019 20:04
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

Upon further review, the example for add_cfe_tables command is almost OK, we should just use a better name than MyTblDefault, that's all.

The selection logic on the implementation side will still work, as the selection logic extracts the basename (filename without path or extension) and uses this to search the "defs" directory for a mission- or cpu-specific version of that table file. The default file, with the path as specified in the argument (either absolute or relative), will be used as a fallback in case there was no mission-specific implementation for the table.

So, the code that is there will work.

But in the context of being a "guide" for best practices, it should be noted that the search logic uses the basename only. Because if this, the suggested name MyTblDefault is not good, for two reasons.

  1. It includes the word "Default" -- once overridden by a mission with a real version, it should keep the same name, but now it becomes a misnomer (not the default anymore).
  2. It is potentially ambiguous in that the name doesn't include the app name at all. If more than one app cloned this same but didn't rename the table file, one would have a name conflict, both in the source tree and the deployment (binary) tree.

For these two reasons, I suggest changing the a name in the documentation to something like ${APPNAME}_default to be clear that the table name should incorporate the app name, to make it remain distinctive when it is no longer in an app-specific subdirectory.

@avan989
Copy link
Contributor Author

avan989 commented Oct 3, 2019

Changed MyTblDefault to ${APPNAME}_default

Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

I suggest the search path behavior get outlined here, and take off the fsw/src in the 3.a. example if it's ignored anyways. Otherwise it implies a specific file, vs the search/override functionality.

So 2. could detail placing the application default table in the fsw/src directory, then note the build system search path for the file of this name - the mission *_defs directory first in case the mission needs to override the default followed by the application fsw/src directory.

Actually curious if it's more complicated than this, where you could prefix the file name with the platform and build different tables for different platforms?

@jphicky could you confirm? Is this general behavior already described somewhere that this file could refer to? (the whole search path, platform prefix business)

@skliper
Copy link
Contributor

skliper commented Oct 8, 2019

I suggest the search path behavior get outlined here, and take off the fsw/src in the 3.a. example if it's ignored anyways. Otherwise it implies a specific file, vs the search/override functionality.

So 2. could detail placing the application default table in the fsw/src directory, then note the build system search path for the file of this name - the mission *_defs directory first in case the mission needs to override the default followed by the application fsw/src directory.

Actually curious if it's more complicated than this, where you could prefix the file name with the platform and build different tables for different platforms?

@jphicky could you confirm? Is this general behavior already described somewhere that this file could refer to? (the whole search path, platform prefix business)

Turns out there isn't a search path. This isn't applicable.

@jphickey
Copy link
Contributor

jphickey commented Oct 8, 2019

I think this is OK now.

But there IS a search path for tables - it is just using the basename only, rather than the full path of whatever is supplied in the command. The sequence is as follows (where TBLWE==table basename without extension):

  1. ${MISSION_DEFS}/tables/${TGT}_${TBLWE}.c
  2. ${MISSION_SOURCE_DIR}/tables/${TGT}_${TBLWE}.c
  3. ${MISSION_DEFS}/tables/${TBLWE}.c
  4. ${MISSION_SOURCE_DIR}/tables/${TBLWE}.c
  5. (passed-in relative path)

The supplied path does come into play, as the fallback at the end, if the search path fails to find anything matching within the mission-specific directories.

@skliper skliper added this to the 3.2.0 milestone Oct 8, 2019
@skliper
Copy link
Contributor

skliper commented Oct 8, 2019

I think this is OK now.

But there IS a search path for tables - it is just using the basename only, rather than the full path of whatever is supplied in the command. The sequence is as follows (where TBLWE==table basename without extension):

  1. ${MISSION_DEFS}/tables/${TGT}_${TBLWE}.c
  2. ${MISSION_SOURCE_DIR}/tables/${TGT}_${TBLWE}.c
  3. ${MISSION_DEFS}/tables/${TBLWE}.c
  4. ${MISSION_SOURCE_DIR}/tables/${TBLWE}.c
  5. (passed-in relative path)

The supplied path does come into play, as the fallback at the end, if the search path fails to find anything matching within the mission-specific directories.

Got it, I suggest this get added to the guide.

@avan989 avan989 closed this Oct 9, 2019
@avan989 avan989 reopened this Oct 9, 2019
@avan989
Copy link
Contributor Author

avan989 commented Oct 9, 2019

added sequence for file table source

@skliper skliper added the CCB:Approved Indicates code approval by CCB label Oct 10, 2019
jphickey added a commit that referenced this pull request Oct 11, 2019
Merge branches 'pull-15', 'pull-20', 'pull-21', 'pull-23'
and 'pull-24' for integration
@jphickey jphickey merged commit bf460d4 into nasa:master Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CCB:Approved Indicates code approval by CCB

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tutorial on using elf2cfetbl

3 participants