Conversation
jfrux
left a comment
There was a problem hiding this comment.
It all looks good. My thoughts are that some of it, although might be more "Python-esque" is not as expressive. Do these improvements offer Python performance improvements when compiled down or are they aesthetic, preference, code-reuse, testability?
Also, should we start using something like a linter to get code-rules for the project to consolidate usage of specific things to keep things consistent moving forward?
Just FYI, I'm all about tapping into code-reuse and testability so I'm definitely NOT saying these changes should not be approved. I'm sure it would become more expressive to me once I'm more familiar with Python syntax as well.
|
I wouldn't assume any significant performance impact either way. This pull was mostly about replacing conditional logic that relied on fingerprints. Going forward this file should rely more on parsing the DBC. I think the use of get() may be what you refer to as "not as expressive." To me, this is pretty standard when working with unknown dictionary keys. I think it's also the cleanest approach when checking a nested dictionary. |
|
@dekerr , we are going to cherry pick some of those changes, but leaving some of the restructure out. In particular |
30c7ca8 bump version to 1.5.3 9403dbe Need to fix wifi test before re-enabling. 0812362 GPS UART fix until boardd is refactored (#294) ffbdb87 python2 -> 3 fixes to pedal flasher (#292) 78b75ef Added build type to release version strings 736c2cb Fixed sending of bytes over PandaSerial 0894b28 Fixed USB power mode on black (#291) 4b3358c patch to be able to switch from EON to PC with a Panda that has EON b… (#290) a95c44a Made setting of NOOUTPUT on no heartbeat more efficient (#287) 9486836 UART instability fix with high interrupt load (#283) 9a9e9d4 Fix usb_power_mode missing initialization (#289) af0960a DFU fix (#288) 70219d7 match safety enum in cereal (#285) a338d39 Fix build for jenkins test 78ef4a6 Stop charge (#284) 5266a40 Fix typo (#286) f4787ec Revert "turn on CDP when ignition switches on (#281)" d37daee Revert "NONE and CLIENT should be the same thing in white/grey pandas" e97b283 NONE and CLIENT should be the same thing in white/grey pandas 8c1df55 turn on CDP when ignition switches on (#281) 847a35d Fix bullet points fac0277 Misra update (#280) 5a04df6 Added description of regression tests to README c4aabae Fixed some python3 bugs in the test scripts and PandaSerial 9af0cb3 Bump version c4ac3d6 Disable GPS load switching on black pandas 078ee58 This is the correct table, actually 578b95e Misra table of coverage added d383a26 bump panda b98ca01 fix sdk build in python3 env (#279) 63d3dc7 Set python3 env before runnign get_sdk, so we know if it fails e951d79 legacy code we don't control can remain python2 11b7151 Merge pull request #276 from commaai/python3 9893a84 Merge pull request #277 from zorrobyte/patch-1 d326869 Revert "revert back esptool to python2 and force to build esptools with python2" 875e760 revert back esptool to python2 and force to build esptools with python2 9c40e62 needed to install python3 ed2ac87 Also moved safety tests to python3 6842b2d move esptool sdk installation before python3 env is set. Kind of a cheat b5a2cab this hopefully fixes build test 6280509 Fixes safety replay 2c220b6 this fixes language regr test fdbe789 use python 3 in Docker container ee1ae4f Better hash print 0de9ef7 Revert "Final 2to3 on the whole repo" c92fd3b Final 2to3 on the whole repo 5f2bc44 better b2a30fd make works! b74005d fix sign.py fe72770 read file as byte and no tab before sleep 32a344e Update README.md 2dc3409 2to3 applied ffa68ef undo unnecessary brackets for print dbc2480 Fix all the prints with 2to3, some need to be undo 5a7aeba xrange is gone 982c4c9 one more python3 env 1e2412a env python -> env python3 git-subtree-dir: panda git-subtree-split: 30c7ca8
30c7ca8 bump version to 1.5.3 9403dbe Need to fix wifi test before re-enabling. 0812362 GPS UART fix until boardd is refactored (commaai#294) ffbdb87 python2 -> 3 fixes to pedal flasher (commaai#292) 78b75ef Added build type to release version strings 736c2cb Fixed sending of bytes over PandaSerial 0894b28 Fixed USB power mode on black (commaai#291) 4b3358c patch to be able to switch from EON to PC with a Panda that has EON b… (commaai#290) a95c44a Made setting of NOOUTPUT on no heartbeat more efficient (commaai#287) 9486836 UART instability fix with high interrupt load (commaai#283) 9a9e9d4 Fix usb_power_mode missing initialization (commaai#289) af0960a DFU fix (commaai#288) 70219d7 match safety enum in cereal (commaai#285) a338d39 Fix build for jenkins test 78ef4a6 Stop charge (commaai#284) 5266a40 Fix typo (commaai#286) f4787ec Revert "turn on CDP when ignition switches on (commaai#281)" d37daee Revert "NONE and CLIENT should be the same thing in white/grey pandas" e97b283 NONE and CLIENT should be the same thing in white/grey pandas 8c1df55 turn on CDP when ignition switches on (commaai#281) 847a35d Fix bullet points fac0277 Misra update (commaai#280) 5a04df6 Added description of regression tests to README c4aabae Fixed some python3 bugs in the test scripts and PandaSerial 9af0cb3 Bump version c4ac3d6 Disable GPS load switching on black pandas 078ee58 This is the correct table, actually 578b95e Misra table of coverage added d383a26 bump panda b98ca01 fix sdk build in python3 env (commaai#279) 63d3dc7 Set python3 env before runnign get_sdk, so we know if it fails e951d79 legacy code we don't control can remain python2 11b7151 Merge pull request commaai#276 from commaai/python3 9893a84 Merge pull request commaai#277 from zorrobyte/patch-1 d326869 Revert "revert back esptool to python2 and force to build esptools with python2" 875e760 revert back esptool to python2 and force to build esptools with python2 9c40e62 needed to install python3 ed2ac87 Also moved safety tests to python3 6842b2d move esptool sdk installation before python3 env is set. Kind of a cheat b5a2cab this hopefully fixes build test 6280509 Fixes safety replay 2c220b6 this fixes language regr test fdbe789 use python 3 in Docker container ee1ae4f Better hash print 0de9ef7 Revert "Final 2to3 on the whole repo" c92fd3b Final 2to3 on the whole repo 5f2bc44 better b2a30fd make works! b74005d fix sign.py fe72770 read file as byte and no tab before sleep 32a344e Update README.md 2dc3409 2to3 applied ffa68ef undo unnecessary brackets for print dbc2480 Fix all the prints with 2to3, some need to be undo 5a7aeba xrange is gone 982c4c9 one more python3 env 1e2412a env python -> env python3 git-subtree-dir: panda git-subtree-split: 30c7ca8
30c7ca8 bump version to 1.5.3 9403dbe Need to fix wifi test before re-enabling. 0812362 GPS UART fix until boardd is refactored (commaai#294) ffbdb87 python2 -> 3 fixes to pedal flasher (commaai#292) 78b75ef Added build type to release version strings 736c2cb Fixed sending of bytes over PandaSerial 0894b28 Fixed USB power mode on black (commaai#291) 4b3358c patch to be able to switch from EON to PC with a Panda that has EON b… (commaai#290) a95c44a Made setting of NOOUTPUT on no heartbeat more efficient (commaai#287) 9486836 UART instability fix with high interrupt load (commaai#283) 9a9e9d4 Fix usb_power_mode missing initialization (commaai#289) af0960a DFU fix (commaai#288) 70219d7 match safety enum in cereal (commaai#285) a338d39 Fix build for jenkins test 78ef4a6 Stop charge (commaai#284) 5266a40 Fix typo (commaai#286) f4787ec Revert "turn on CDP when ignition switches on (commaai#281)" d37daee Revert "NONE and CLIENT should be the same thing in white/grey pandas" e97b283 NONE and CLIENT should be the same thing in white/grey pandas 8c1df55 turn on CDP when ignition switches on (commaai#281) 847a35d Fix bullet points fac0277 Misra update (commaai#280) 5a04df6 Added description of regression tests to README c4aabae Fixed some python3 bugs in the test scripts and PandaSerial 9af0cb3 Bump version c4ac3d6 Disable GPS load switching on black pandas 078ee58 This is the correct table, actually 578b95e Misra table of coverage added d383a26 bump panda b98ca01 fix sdk build in python3 env (commaai#279) 63d3dc7 Set python3 env before runnign get_sdk, so we know if it fails e951d79 legacy code we don't control can remain python2 11b7151 Merge pull request commaai#276 from commaai/python3 9893a84 Merge pull request commaai#277 from zorrobyte/patch-1 d326869 Revert "revert back esptool to python2 and force to build esptools with python2" 875e760 revert back esptool to python2 and force to build esptools with python2 9c40e62 needed to install python3 ed2ac87 Also moved safety tests to python3 6842b2d move esptool sdk installation before python3 env is set. Kind of a cheat b5a2cab this hopefully fixes build test 6280509 Fixes safety replay 2c220b6 this fixes language regr test fdbe789 use python 3 in Docker container ee1ae4f Better hash print 0de9ef7 Revert "Final 2to3 on the whole repo" c92fd3b Final 2to3 on the whole repo 5f2bc44 better b2a30fd make works! b74005d fix sign.py fe72770 read file as byte and no tab before sleep 32a344e Update README.md 2dc3409 2to3 applied ffa68ef undo unnecessary brackets for print dbc2480 Fix all the prints with 2to3, some need to be undo 5a7aeba xrange is gone 982c4c9 one more python3 env 1e2412a env python -> env python3 git-subtree-dir: panda git-subtree-split: 30c7ca8
* MSPA: Lat plan deprecation prerequisites * bump cereal
* Revert "UI: check params every 5 seconds" * add comment
-removed repeated code
-lowered dependency on fingerprint matching
-improved readability
-added comments