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.

Sign up for GitHub

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

Jump to bottom

Fix #36 #38 #40 #41, Overhaul tbl CRC #42

Merged
merged 1 commit into from
Mar 5, 2021
Merged

Fix #36 #38 #40 #41, Overhaul tbl CRC #42

merged 1 commit into from
Mar 5, 2021

Conversation

zanzaben
Copy link
Contributor

@zanzaben zanzaben commented Mar 4, 2021

Describe the contribution
Fixes #36
Fixes #38
Fixes #40
Fixes #41
All these issues were starting to overlap with each other too much so I made them all one file. This should clean up all the issues with the tblCRCTool

Testing performed
Build and run unit test

Expected behavior changes
No impact to behavior

System(s) tested on
Ubuntu 20.04

Additional context
Replaces #37 and #39

Contributor Info - All information REQUIRED for consideration of pull request
Alex Campbell GSFC

This was referenced Mar 4, 2021
Closed
Closed
@zanzaben zanzaben requested a review from jphickey March 4, 2021 14:37
@zanzaben zanzaben changed the title WIP Fix #36 #38 #40 #41, Overhaul tbl CRC Fix #36 #38 #40 #41, Overhaul tbl CRC Mar 4, 2021
@zanzaben zanzaben marked this pull request as ready for review March 4, 2021 14:38
@zanzaben zanzaben requested a review from skliper March 4, 2021 14:39
jphickey
jphickey requested changes Mar 4, 2021
View reviewed changes
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.

This still needs some changes... (sorry!)

cfe_ts_crc.c Show resolved Hide resolved
cfe_ts_crc.c Show resolved Hide resolved
cfe_ts_crc.c Outdated Show resolved Hide resolved
cfe_ts_crc.c Outdated Show resolved Hide resolved
@zanzaben zanzaben force-pushed the crc_overhaul branch 2 times, most recently from 10db681 to 05f8652 Compare March 5, 2021 17:04
jphickey
jphickey approved these changes Mar 5, 2021
View reviewed changes
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.

Looks Good!

@jphickey
Copy link
Contributor

jphickey commented Mar 5, 2021

I would still like to see the hardcoded 100 get fixed to be sizeof(buffer) but its not the end of the world in a tool like this.

@zanzaben
Fix nasa#36 nasa#38 nasa#40 nasa#41
558b20c 
36: Handle read failure
38: handle lseek error
40: use exit (1)
41: fix EOF check
@skliper skliper added this to the 1.3.0 milestone Mar 5, 2021
@skliper skliper self-assigned this Mar 5, 2021
@skliper skliper mentioned this pull request Mar 5, 2021
Merged
@skliper skliper merged commit 262c396 into nasa:main Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skliper skliper skliper approved these changes

@jphickey jphickey jphickey approved these changes

Assignees

@skliper skliper

Labels
CCB:FastTrack
Projects
None yet
Milestone
Caelum
3 participants
@zanzaben @jphickey @skliper