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

tblCRCTool hardcodes table file header size #3

Closed
skliper opened this issue Jun 24, 2019 · 8 comments · Fixed by #23 or #25
Closed

tblCRCTool hardcodes table file header size #3

skliper opened this issue Jun 24, 2019 · 8 comments · Fixed by #23 or #25
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Jun 24, 2019

Maintenance issue with hardcoded header size in tblCRCTool

@skliper skliper added the enhancement New feature or request label Jun 24, 2019
@avan989
Copy link

avan989 commented Aug 29, 2019

Can you give an example of what you are referring to?

@skliper
Copy link
Contributor Author

skliper commented Aug 29, 2019

The issue is about skipSize hardcoded to 116. Not clear where that number comes from, but likely related to CFE_FS_Header_t and/or CFE_TBL_File_Hdr_t (as seen in the CFE_TBL_LoadCmd from cfe/fsw/cfe-core/src/tbl/cfe_tbl_task_cmds.c) Take a look at that code and see if you can figure it out.

skliper added a commit that referenced this issue Sep 10, 2019
Fix #3, hardcode table file header size
skliper added a commit that referenced this issue Sep 10, 2019
Reviewed and approved at 2019-09-04 CCB
skliper added a commit that referenced this issue Sep 10, 2019
Reviewed and approved at 2019-09-04 CCB
@jphickey
Copy link
Contributor

I found that the proposed fix for this is breaking the ability to build this tool as a standalone item, which I thought was the whole point of it -- to have a standalone tool independent of CFE or elf2cfetbl that can re-compute a compatible CRC value.

I recommend reverting this change set.

@jphickey
Copy link
Contributor

Upon further review, I don't see the number "116" as random at all; The CFE TBL header is defined as containing certain binary fields of a certain size and this actually should be fixed in this particular case, just like the size of e.g. a UDP or Ethernet header is defined and fixed. Using "sizeof()" here actually introduces more potential problems than existed before.

@skliper
Copy link
Contributor Author

skliper commented Sep 12, 2019

@jphickey I approach the use case for this tool differently. We need a ground tool that calculates the CRC such that we can verify the reported CRC from cFE is as expected. There's no requirement for this tool to be completely stand alone. In my view this now does exactly what I want, and removing the hardcoded header side increases maintainability vs using a "magic number".

The point is not to checksum everything but the first 116 bytes. The point is to calculate the checksum in the same way the cFS will do it in flight, to be able to confirm the file on the ground matches what was loaded.

@skliper
Copy link
Contributor Author

skliper commented Sep 12, 2019

@jphickey I'd actually prefer if it was built with the rest of the system (with cmake), like cmdUtil and elf2cfetbl. It's not really stand alone.

@skliper
Copy link
Contributor Author

skliper commented Sep 12, 2019

From #8, I concur with the improvements (add CMakelists.txt, fix nasa/cFE#25, remove Makefile, and clean up header includes if possible). Then we'll re-review.

@skliper skliper assigned jphickey and unassigned avan989 Sep 12, 2019
@skliper
Copy link
Contributor Author

skliper commented Oct 2, 2019

nasa/cFE#25 has been merged, this can now be implemented.

jphickey added a commit to jphickey/tblCRCTool that referenced this issue Oct 30, 2020
sizeof() will keep this tool in sync if the size of the CFE
file or table header should ever change.

Note this also implicitly restricts this tool to the CFE build
environment where these headers are available.  As a result
this will no longer be buildable as a fully standalone/external
tool.
@astrogeco astrogeco added this to the 1.3.0 milestone Dec 1, 2020
astrogeco added a commit that referenced this issue Dec 1, 2020
Fix #3, use sizeof() for header sizes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants