Exposing padding support in uds discovery. #100
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Exposing padding support in uds discovery. This also adds opts --no-pad and --padding to uds discovery subcommand.
kasperkarlsson
requested changes
Jan 16, 2024
| @@ -169,7 +169,7 @@ def is_valid_response(message): | |||
|
|
|||
| found_arbitration_ids = [] | |||
|
|
|||
| with IsoTp(None, None) as tp: | |||
| with IsoTp(None, None, padding_value=None) as tp: | |||
There was a problem hiding this comment.
If possible the ISO-TP support for padding (implemented in 3713cec) should be used here, rather than turning it off and reinventing the ISO-TP padding on a higher protocol level inside the UDS module. This might however be more applicable for a future rewrite of the module which does not fiddle with ISO-TP internals 🙂 Any thoughts/opinions on this are welcome!
| @@ -110,7 +110,7 @@ | |||
|
|
|||
|
|
|||
| def uds_discovery(min_id, max_id, blacklist_args, auto_blacklist_duration, | |||
| delay, verify, print_results=True): | |||
| delay, verify, padding, print_results=True): | |||
There was a problem hiding this comment.
Adding a required positional argument to uds_discovery without a default value will break all existing code using it, including (but not necessarily limited to) two tests in the CC test suite:
$ cc.py -i vcan0 test
-------------------
CARING CARIBOU v0.4
-------------------
Loading module 'test'
Running tests using CAN interface 'vcan0'
test_create_iso_14229_1 (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_ecu_reset_failure_on_invalid_reset_type (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_ecu_reset_failure_suppress_positive_response (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_ecu_reset_success (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_ecu_reset_success_suppress_positive_response (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_read_data_by_identifier_failure (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_read_data_by_identifier_success (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_read_memory_by_address_failure_on_invalid_length (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_read_memory_by_address_success (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_write_data_by_identifier_failure (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_write_data_by_identifier_success (test_iso_14229_1.DiagnosticsOverIsoTpTestCase) ... ok
test_create_iso_15765_2 (test_iso_15765_2.IsoTpTestCase) ... ok
test_fail_too_long_message (test_iso_15765_2.IsoTpTestCase) ... ok
test_multi_frame_long_message (test_iso_15765_2.IsoTpTestCase) ... ok
test_multi_frame_two_frames (test_iso_15765_2.IsoTpTestCase) ... ok
test_single_frame (test_iso_15765_2.IsoTpTestCase) ... ok
ok0101ump_dids (test_module_uds.UdsModuleTestCase) ... 0x0000
test_ecu_reset_hard_reset_success (test_module_uds.UdsModuleTestCase) ... ok
test_ecu_reset_unsupported_reset_type_failure (test_module_uds.UdsModuleTestCase) ... ok
test_security_access_request_seed_invalid_level (test_module_uds.UdsModuleTestCase) ... ok
test_security_access_request_seed_send_key_success (test_module_uds.UdsModuleTestCase) ... ok
test_service_discovery (test_module_uds.UdsModuleTestCase) ... ok
test_service_discovery_empty_range (test_module_uds.UdsModuleTestCase) ... ok
test_uds_discovery (test_module_uds.UdsModuleTestCase) ... ERROR
test_uds_discovery_blacklist (test_module_uds.UdsModuleTestCase) ... ERROR
test_parse_candump_line (test_send.SendFileParserTestCase) ... ok
test_parse_pythoncan_line_v_20 (test_send.SendFileParserTestCase) ... ok
test_parse_pythoncan_line_v_21 (test_send.SendFileParserTestCase) ... ok
test_parse_pythoncan_line_v_21_flags (test_send.SendFileParserTestCase) ... ok
test_parse_pythoncan_line_v_30_channel (test_send.SendFileParserTestCase) ... ok
test_parse_pythoncan_line_v_30_flags (test_send.SendFileParserTestCase) ... ok
======================================================================
ERROR: test_uds_discovery (test_module_uds.UdsModuleTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/local/lib/python3.10/dist-packages/caringcaribou-0.4-py3.10.egg/caringcaribou/tests/test_module_uds.py", line 36, in test_uds_discovery
result = uds.uds_discovery(min_id=start_arb_id,
TypeError: uds_discovery() missing 1 required positional argument: 'padding'
======================================================================
ERROR: test_uds_discovery_blacklist (test_module_uds.UdsModuleTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/local/lib/python3.10/dist-packages/caringcaribou-0.4-py3.10.egg/caringcaribou/tests/test_module_uds.py", line 58, in test_uds_discovery_blacklist
result = uds.uds_discovery(min_id=start_arb_id,
TypeError: uds_discovery() missing 1 required positional argument: 'padding'
----------------------------------------------------------------------
Ran 31 tests in 0.899s
FAILED (errors=2)
It also seems to be missing in the docstring.
| @@ -287,11 +287,16 @@ def __uds_discovery_wrapper(args): | |||
| delay = args.delay | |||
| verify = not args.skipverify | |||
| print_results = True | |||
| padding=args.padding | |||
There was a problem hiding this comment.
The formatting of this block does not adhere to PEP 8, both in terms of spacing and the double newlines at the end
| @@ -837,7 +842,7 @@ def __auto_wrapper(args): | |||
| try: | |||
| arb_id_pairs = uds_discovery(min_id, max_id, blacklist, | |||
| auto_blacklist_duration, | |||
| delay, verify, print_results) | |||
| delay, verify, padding, print_results) | |||
There was a problem hiding this comment.
padding is undefined here, causing the whole __auto_wrapper function to break:
$ cc.py -i vcan0 uds auto
-------------------
CARING CARIBOU v0.4
-------------------
Loading module 'uds'
Traceback (most recent call last):
File "/usr/local/bin/cc.py", line 33, in <module>
sys.exit(load_entry_point('caringcaribou==0.4', 'console_scripts', 'cc.py')())
File "/usr/local/lib/python3.10/dist-packages/caringcaribou-0.4-py3.10.egg/caringcaribou/caringcaribou.py", line 125, in main
cc_mod.module_main(args.module_args)
File "/usr/local/lib/python3.10/dist-packages/caringcaribou-0.4-py3.10.egg/caringcaribou/modules/uds.py", line 1347, in module_main
args.func(args)
File "/usr/local/lib/python3.10/dist-packages/caringcaribou-0.4-py3.10.egg/caringcaribou/modules/uds.py", line 845, in __auto_wrapper
delay, verify, padding, print_results)
NameError: name 'padding' is not defined
|
Many thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Exposing padding support in uds discovery. This also adds opts --no-pad and --padding to uds discovery subcommand.