Replace Python CRC implementations with satellites.crc (fixes #731)#798
Replace Python CRC implementations with satellites.crc (fixes #731)#798pushkardube wants to merge 13 commits intodaniestevez:mainfrom
Conversation
Remove the 256-entry lookup table and hand-rolled loop in check_eseo_crc.py, delegating to the generic C++ crc class (poly=0x1021, init=0x0000, final_xor=0x0000, no reflection), which is already available as satellites.crc. Part of daniestevez#731
Migrate check_cc11xx_crc, check_tt64_crc, check_crc16_ccitt_false, ngham_check_crc, sx12xx_check_crc, and crc32c to use the generic satellites.crc C++ engine instead of hand-rolled 256-entry tables. All 11 qa_crc tests pass. Part of daniestevez#731.
… C++ crc binding Part of daniestevez#731
|
@daniestevez Please review |
daniestevez
left a comment
There was a problem hiding this comment.
It looks like we can replace all of the affected blocks by suitable instances of the CRC Check and CRC Append blocks.
| _crc_fn = _crc_module(16, 0x1021, 0xffff, 0x0, False, False) | ||
|
|
||
|
|
||
| def crc(data): |
There was a problem hiding this comment.
It looks like the check_ao40_uncoded_crc block could be deprecated and replaced by crc16_ccitt_false from crcs.py.
There was a problem hiding this comment.
This comment is outstanding. If ao40_uncoded_crc.py is only used in blocks that are going to be deprecated, it can be deprecated as well an marked with a comment for future removal.
python/check_cc11xx_crc.py
Outdated
| 0x8213, 0x0216, 0x021c, 0x8219, 0x0208, 0x820d, 0x8207, 0x0202] | ||
| # CRC-16/IBM: poly=0x8005, init=0xffff, final_xor=0x0, input_reflected=False, | ||
| # result_reflected=False | ||
| _crc_fn = _crc_module(16, 0x8005, 0xffff, 0x0, False, False) |
There was a problem hiding this comment.
It looks like the block check_cc11xx_crc could be deprecated and replaced by crc16_cc11xx from crcs.py.
There was a problem hiding this comment.
Hii @daniestevez, I have basically converted the original gr.basic_block with its own Python CRC logic into a thin gr.hier_block2 that delegates entirely to crc16_cc11xx() from crcs.py... If this direction looks correct I could proceed similarly for the other files as well.
And thank you for the review.
There was a problem hiding this comment.
Why? There is no need to modify a block that is going to be deprecated. It will eventually be deleted. To mark it as deprecated, the most important thing is to mark it in the GRC YAML file (there are probably other examples of deprecated blocks in gr-satellites). The other thing we need to do is to replace any usage of the deprecated block within gr-satellites by the replacement block.
There was a problem hiding this comment.
This comment is outstanding. Since check_cc11xx_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/check_crc16_ccitt_false.py
Outdated
| 0x6e17, 0x7e36, 0x4e55, 0x5e74, 0x2e93, 0x3eb2, 0x0ed1, 0x1ef0] | ||
| # CRC-16/CCITT-FALSE: poly=0x1021, init=0xffff, final_xor=0x0, | ||
| # input_reflected=False, result_reflected=False | ||
| _crc_fn = _crc_module(16, 0x1021, 0xffff, 0x0, False, False) |
There was a problem hiding this comment.
It looks like the block check_crc16_ccitt_false could be deprecated and replaced by crc16_ccitt_false from crcs.py.
There was a problem hiding this comment.
This comment is outstanding. Since check_crc16_ccitt_false is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/check_eseo_crc.py
Outdated
| 0x6e17, 0x7e36, 0x4e55, 0x5e74, 0x2e93, 0x3eb2, 0x0ed1, 0x1ef0] | ||
| # CRC-16/CCITT-ZERO: poly=0x1021, init=0x0000, final_xor=0x0000, | ||
| # input_reflected=False, result_reflected=False | ||
| _crc_fn = _crc_module(16, 0x1021, 0x0000, 0x0000, False, False) |
There was a problem hiding this comment.
It looks like the block check_eseo_crc could be deprecated and replaced by crc16_ccitt_zero from crcs.py.
There was a problem hiding this comment.
This comment is outstanding. Since check_eseo_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/check_tt64_crc.py
Outdated
| 0x8201, 0x42c0, 0x4380, 0x8341, 0x4100, 0x81c1, 0x8081, 0x4040] | ||
| # CRC-16/ARC: poly=0x8005, init=0x0, final_xor=0x0, input_reflected=True, | ||
| # result_reflected=True | ||
| _crc_fn = _crc_module(16, 0x8005, 0x0, 0x0, True, True) |
There was a problem hiding this comment.
It looks like the block check_tt64_crc could be deprecated and replaced by crc16_arc from crcs.py.
There was a problem hiding this comment.
This comment is outstanding. Since check_tt64_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/crc32c.py
Outdated
| finalized 32-bit checksum as long | ||
| """ | ||
| return crc ^ 0xffffffff | ||
| _crc_fn = _crc_module(32, 0x1EDC6F41, 0xffffffff, 0xffffffff, True, True) |
There was a problem hiding this comment.
It looks like the block check_crc could directly use _crc_module like this, and the block append_crc32c could be deprecated and replaced by an instance of CRC Append built with these parameters.
There was a problem hiding this comment.
This comment is outstanding. As this file becomes unused due to changes in append_crc32c.py and check_crc.py, it can be completely deleted.
python/hdlc.py
Outdated
|
|
||
| from . import crc as _crc_module | ||
|
|
||
| _crc_fn = _crc_module(16, 0x1021, 0xffff, 0xffff, True, True) |
There was a problem hiding this comment.
This file is only used in check_crc16_ccitt.py, which can be deprecated and replaced by the corresponding block in crcs.py.
There was a problem hiding this comment.
This comment is outstanding. Since check_crc16_ccitt is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
There was a problem hiding this comment.
So hdlc_deframer.py, pwsat2_submitter.py, and k2sat_deframer.py import hdlc but don't actually use anything from it, the import appears unused.
Only hdlc_framer.py uses hdlc.flag which is (flag = bytes([0] + 6*[1] + [0])) just one variable.
Do I remove the import and mark the file deprecated??
I have reverted the files to their original state
There was a problem hiding this comment.
If hdlc_deframer.py, pwsat2_submitter.py, and k2sat_deframer.py don't use anything from hdlc, it's fine to remove the import.
There was a problem hiding this comment.
Ok sir, and what about hdlc_framer.py? The usage is very minor if this could be changed then we could properly mark hdlc.py as deprecated in the GRC YAML file
There was a problem hiding this comment.
The flag in hdlc.py can be moved to hdlc_framer.py if it isn't used anywhere. The file hdlc.py can then be deleted. It is not a block, so it is not in gr-satellite's public API, and it can be removed at any point in time. It has no GRC YAML file.
There was a problem hiding this comment.
I have made the changes. Please review
python/ngham_check_crc.py
Outdated
| 0x7bc7, 0x6a4e, 0x58d5, 0x495c, 0x3de3, 0x2c6a, 0x1ef1, 0x0f78] | ||
| # CRC-16/X-25: poly=0x1021, init=0xffff, final_xor=0xffff, | ||
| # input_reflected=True, result_reflected=True | ||
| _crc_fn = _crc_module(16, 0x1021, 0xffff, 0xffff, True, True) |
There was a problem hiding this comment.
It looks like the ngham_check_crc block can be deprecated and replaced by crc16_ccitt_x25 from crcs.py.
There was a problem hiding this comment.
This comment is outstanding. Since ngham_check_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/sx12xx_check_crc.py
Outdated
| self.verbose = verbose | ||
| self.initial = initial | ||
| self.final = final | ||
| self._crc_fn = _crc_module(16, 0x1021, initial, final, False, False) |
There was a problem hiding this comment.
The block sx12xx_check_crc does not seem to be used by other blocks. It can be deprecated, since its usage is covered by the CRC Check block and the "presets" from crcs.py.
There was a problem hiding this comment.
This comment is outstanding. Since sx12xx_check_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
…cc11xx(); add CRC-16/CC11xx test
Block is already marked deprecated in GRC YAML and has no usages in any .grc flowgraph, so modifying it is unnecessary.
…ppend_crc32c Removes indirection through crc32c.py; both blocks now instantiate _crc_fn directly with CRC-32C parameters. Drops unused numpy import from append_crc32c. check_cc11xx_crc, check_ao40_uncoded_crc, sx12xx_check_crc: no changes needed; GRC YAMLs already deprecated and no internal usages remain. check_tt64_crc: no changes needed; GRC YAML already deprecated and tt64_deframer/ao40_fec_deframer already use crcs.crc16_arc() directly since upstream commit 2c3d9f2, not through the deprecated block.
…from check_eseo_crc Three files (check_swiatowid_crc, tubix20_reframer, mobitex_to_datablocks) imported crc16_ccitt_zero from check_eseo_crc.py. Redirect them to use the crc module directly via _crc_fn = crc(16, 0x1021, 0x0000, 0x0000, False, False), matching the pattern already used by check_eseo_crc itself. Remove the now-orphaned crc16_ccitt_zero helper and unused numpy import from check_eseo_crc.py. No changes needed in hdlc.py (already uses _crc_module directly), ngham_check_crc.py (no external callers of its helper), or check_crc16_ccitt_false.py (GRC block already deprecated).
|
Hii, apologies for the delay, had some college exams but i have made the following changes and would appreciate a re-review
No changes were needed in:
|
daniestevez
left a comment
There was a problem hiding this comment.
It seems that there is a miscommunication regarding the scope of these changes. I have left comments where appropriate, but the summary is:
-
Many blocks can be replaced by the generic CRC Append and CRC Check block with suitable parameters. All these blocks can be marked as deprecated. No changes are needed to the code of these blocks, other than marking for deprecation, since the code will be deleted in a few releases. If any of these blocks is used within gr-satellites (and maybe this doesn't happen, but we need to look at deframers, etc.), then their uses should be replaced by the suitable CRC Append and CRC Check instances. This hasn't been done correctly in the PR currently.
-
There are some uses of CRC calculations that cannot be replaced with CRC Append and CRC Check. These are
append_crc32c, andcheck_crc(because they have an option about whether to include the CSP header), andmobitex_to_datablocks(because it computes the CRC of parts of the message). Here the right choice is to make an instance of the CRC class and call thecompute()method of that instance. This has been done correctly in the PR.
| _crc_fn = _crc_module(16, 0x1021, 0xffff, 0x0, False, False) | ||
|
|
||
|
|
||
| def crc(data): |
There was a problem hiding this comment.
This comment is outstanding. If ao40_uncoded_crc.py is only used in blocks that are going to be deprecated, it can be deprecated as well an marked with a comment for future removal.
python/check_cc11xx_crc.py
Outdated
| 0x8213, 0x0216, 0x021c, 0x8219, 0x0208, 0x820d, 0x8207, 0x0202] | ||
| # CRC-16/IBM: poly=0x8005, init=0xffff, final_xor=0x0, input_reflected=False, | ||
| # result_reflected=False | ||
| _crc_fn = _crc_module(16, 0x8005, 0xffff, 0x0, False, False) |
There was a problem hiding this comment.
This comment is outstanding. Since check_cc11xx_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/check_crc16_ccitt_false.py
Outdated
| 0x6e17, 0x7e36, 0x4e55, 0x5e74, 0x2e93, 0x3eb2, 0x0ed1, 0x1ef0] | ||
| # CRC-16/CCITT-FALSE: poly=0x1021, init=0xffff, final_xor=0x0, | ||
| # input_reflected=False, result_reflected=False | ||
| _crc_fn = _crc_module(16, 0x1021, 0xffff, 0x0, False, False) |
There was a problem hiding this comment.
This comment is outstanding. Since check_crc16_ccitt_false is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/check_eseo_crc.py
Outdated
| 0x6e17, 0x7e36, 0x4e55, 0x5e74, 0x2e93, 0x3eb2, 0x0ed1, 0x1ef0] | ||
| # CRC-16/CCITT-ZERO: poly=0x1021, init=0x0000, final_xor=0x0000, | ||
| # input_reflected=False, result_reflected=False | ||
| _crc_fn = _crc_module(16, 0x1021, 0x0000, 0x0000, False, False) |
There was a problem hiding this comment.
This comment is outstanding. Since check_eseo_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/check_swiatowid_crc.py
Outdated
| from . import crc as _crc_module | ||
|
|
||
| _crc_fn = _crc_module(16, 0x1021, 0x0000, 0x0000, False, False) |
There was a problem hiding this comment.
This shows that check_swiatowid_crc can be replaced by crc16_ccitt_zero from crcs.py. Therefore, no changes to this file should be done other than marking for deprecation.
There was a problem hiding this comment.
Reverted check_swiatowid_crc.py to match main since it's deprecated and shouldn't have code changes. To keep its import working, restored crc16_ccitt_zero() in check_eseo_crc.py as a small wrapper around the new _crc_fn binding.
There was a problem hiding this comment.
hello @daniestevez, Are these changes correct?? Please check it and let me know if any more changes are required.
And is it a good idea to add runtime deprecation warnings in all the other deprecated files?
There was a problem hiding this comment.
I restate what I said in #798 (review) plus all the comment threads that are open.
There was a problem hiding this comment.
Ohh I realize that my understanding was quite wrong. Please correct me if I'm wrong... but basically first I have to revert all the deprecated files to their main's version (remove even the C++ binding), just mark them as deprecated and replace their usage in other files and second if an deprecated file uses another deprecated then also do not make any changes as they will be deleted at the same time like in the case of check_swiatowid_crc.py and check_eseo_crc.py and lastly delete crc32c.py because it becomes unused now.
There was a problem hiding this comment.
I don't think that crc32c.py becomes unused until the deprecated blocks are eventually removed in the future.
We deprecate blocks (which is what is in the gr-satellites public API), not files. If any of the deprecated blocks is used anywhere in gr-satellites (it might not), then its usage should be replaced.
There was a problem hiding this comment.
Ok i think i understand... just to confirm though if the blocks are deprecated i should keep their python crc implementation only and about the crc32c.py #798 (comment) said it could be deleted which is why i asked.
There was a problem hiding this comment.
I don't think that crc32c.py can be deleted right away, as it is used by some of the blocks that are getting deprecated. What I implied in that comment was that it would be deleted eventually, when the deprecated blocks are deleted.
python/check_tt64_crc.py
Outdated
| 0x8201, 0x42c0, 0x4380, 0x8341, 0x4100, 0x81c1, 0x8081, 0x4040] | ||
| # CRC-16/ARC: poly=0x8005, init=0x0, final_xor=0x0, input_reflected=True, | ||
| # result_reflected=True | ||
| _crc_fn = _crc_module(16, 0x8005, 0x0, 0x0, True, True) |
There was a problem hiding this comment.
This comment is outstanding. Since check_tt64_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/crc32c.py
Outdated
| finalized 32-bit checksum as long | ||
| """ | ||
| return crc ^ 0xffffffff | ||
| _crc_fn = _crc_module(32, 0x1EDC6F41, 0xffffffff, 0xffffffff, True, True) |
There was a problem hiding this comment.
This comment is outstanding. As this file becomes unused due to changes in append_crc32c.py and check_crc.py, it can be completely deleted.
python/hdlc.py
Outdated
|
|
||
| from . import crc as _crc_module | ||
|
|
||
| _crc_fn = _crc_module(16, 0x1021, 0xffff, 0xffff, True, True) |
There was a problem hiding this comment.
This comment is outstanding. Since check_crc16_ccitt is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/ngham_check_crc.py
Outdated
| 0x7bc7, 0x6a4e, 0x58d5, 0x495c, 0x3de3, 0x2c6a, 0x1ef1, 0x0f78] | ||
| # CRC-16/X-25: poly=0x1021, init=0xffff, final_xor=0xffff, | ||
| # input_reflected=True, result_reflected=True | ||
| _crc_fn = _crc_module(16, 0x1021, 0xffff, 0xffff, True, True) |
There was a problem hiding this comment.
This comment is outstanding. Since ngham_check_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
python/sx12xx_check_crc.py
Outdated
| self.verbose = verbose | ||
| self.initial = initial | ||
| self.final = final | ||
| self._crc_fn = _crc_module(16, 0x1021, initial, final, False, False) |
There was a problem hiding this comment.
This comment is outstanding. Since sx12xx_check_crc is going to be deprecated, there is no need to do any changes to this file (other than marking as deprecated).
…crc to main check_eseo_crc: restore crc16_ccitt_zero() as a thin wrapper over _crc_fn so that check_swiatowid_crc (deprecated, no code changes) can continue to import it. The lookup-table removal from an earlier commit is kept. check_swiatowid_crc: revert to main — deprecated blocks receive no code changes.
Reverted to main state: - python/ao40_uncoded_crc.py - python/check_cc11xx_crc.py - python/check_crc16_ccitt_false.py - python/check_eseo_crc.py - python/check_tt64_crc.py - python/crc32c.py - python/ngham_check_crc.py - python/sx12xx_check_crc.py - python/hdlc.py (crc_ccitt only used by deprecated check_crc16_ccitt)
Nine Python files had their own manual CRC implementations (lookup tables and bit-level loops).
These were removed and replaced with calls to the existing C++
satellites.crcbinding.Files updated
Tests
All 11
qa_crctests pass successfully.