-
Notifications
You must be signed in to change notification settings - Fork 5
add CCSDS TC r=1/2 codes #4
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
base: main
Are you sure you want to change the base?
Conversation
|
Fixed the formatting issues. |
daniestevez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing this!
I have a few minor suggestions about coding style that I have written in comments. Besides this, the module comment at the top of ccsds.rs needs to be updated to mention the TC codes (since now this module also implements TC codes), and I would suggest to put your implementation of the TC codes below the implementation of the C2 code (rather than between AR4JA and C2), so that the structure of the file is first everything in the TM book (which is both AR4JA and C2), and then everything in the TC book.
Do these suggestions sound good?
I have compared your implementation to the TC Blue Book and it looks good to me. I don't have known-good alists for these codes, but I don't have any reason to think that there are mistakes in your code (I did check all the exponents of the
I've seen that CI was failing on this PR because of some unrelated clippy lints. These have appeared in a new version of rustc, and I have fixed them in #5, so you should rebase your PR branch to the latest main to get these fixed.
| } | ||
|
|
||
| // Section 4.2.2 in CCSDS 231.0-B-4 | ||
| fn get_tc_info_map(size: TCInfoSize) -> [[Vec<u16>; TCCode::COL_BLOCKS]; TCCode::ROW_BLOCKS] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing Vec<u16> to Vec<usize> here would remove the need of doing let circ = usize::from(circ) above, so it seems like a good idea. There is not much reason to use u16 even if all the numbers here fit into a u16 (they also fit into a u8).
| /// Constructs the parity check matrix for the code. | ||
| pub fn h(&self) -> SparseMatrix { | ||
| // N = k/4 | ||
| let n: usize = match self.k { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the type annotation : usize here is not absolutely necessary.
| } | ||
|
|
||
| // Section 4.2.2 in CCSDS 231.0-B-4 | ||
| fn get_tc_info_map(size: TCInfoSize) -> [[Vec<u16>; TCCode::COL_BLOCKS]; TCCode::ROW_BLOCKS] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this function could be a (private) method of TCCode and use self.k instead of having a size argument.
Thanks so much for this awesome project! I've added the TC LDPC codes from the CCSDS Sync and Channel Coding recommendations blue book, CCSDS 231.0-B-4. I believe I've specified this correctly, but another pair of eyes wouldn't hurt.