I am creating a Python 3.8 script that executes a series of tests that reads and writes information to and from a CAN bus network. I'm using the python-can and the cantools packages. I've managed to transmit and receive data with small functions individually without issue.
I feel I'm not creating the proper "Pythonic" script architecture that allows my script to use all functions, instances and variables between modules.
Intended architecture goal:
main.py
- contains the CAN bus transmit and receive functions, performs initialization and cycles through each test case located in thetest_case.py
module.test_case.py
- stores all test cases. Where each test case is a stand alone function. Each test case must be an isolated function so that if one test needs to be removed or a new test added the script won't break. Additionally, there will likely be dozens maybe hundreds of test cases. So I'd like to keep them isolated to one module for code cleanliness.test_thresholds.py
- would keep all the pass/fail threshold variables that each test case intest_case.py
will refer to.
Problems / Questions:
main.py
instantiates a CAN bus objectbus = can.Bus(bustype='pcan', channel='PCAN_USBBUS1', bitrate=500000)
this object is required for the transmit and receive functions. Because the transmit and receive functions are inmain.py
, this wasn't a problem until I tried to execute a test case in thetest_case.py
module which references the transmit and receive functions inmain.py
Once I attempted to execute a test case an error occurred stating that the
receive()
function being called from thetest_case.py
moduleNameError: name 'bus' is not defined
I understand this astest_case.py
does not know what thebus
instance is. This problem also occurs with mycan
instances. I havefrom main import *
in mytest_case.py
I know this is bad but I am not sure how elsetest_cases.py
will use the transmit and receive functions along with thebus
andcan
instancesHow can I share that instances between modules? What are the best practices here? I have tried to go over several posts on Stack Overflow regarding passing objects (I think that's what my problem is) but none of them seem to answer what I'm looking for.
Is my architecture design acceptable? I'm new to designing larger scripts and I want to make sure I am doing it effectively/proper so that it can scale.
Note: I've cut down a lot of my code to make it more readable here. It may not run if you try it.
main.py
import can
import cantools
import test_cases.test_cases # import all test cases
import time
# sending a single CAN message
def single_send(message):
try:
bus.send(message)
except can.CanError:
print("Message NOT sent")
# receive a message and decode payload
def receive(message, signal):
_counter = 0
try:
while True:
msg = bus.recv(1)
try:
if msg.arbitration_id == message.arbitration_id:
message_data = db.decode_message(msg.arbitration_id, msg.data)
signal_data = message_data.get(signal)
return signal_data
except AttributeError:
_counter += 1
if _counter == 5:
print("CAN Bus InActive")
break
finally:
if _counter == 5:
# reports false if message fails to be received
return False
def main():
for name, tests in test_cases.test_cases.__dict__.items():
if name.startswith("tc") and callable(tests):
tests()
if __name__ == "__main__":
bus = can.Bus(bustype='pcan', channel='PCAN_USBBUS1', bitrate=500000)
db = cantools.db.load_file('C:\\Users\\tw\\Desktop\\dbc_file.dbc')
verbose_log = open("verbose_log.txt", "a")
main()
bus.shutdown()
verbose_log.close()
test_case.py
from test_thresholds.test_thresholds import *
from main import * # to use the single_send and receive functions in main
def tc_1():
ct = receive(0x300, 'ct_signal') # this is where the issue occurs. receive expects the bus instance
message = can.Message(arbitration_id=0x303, data=1)
if (ct > ct_min) and (ct < ct_max):
verbose_log.write("PASS")
else:
verbose_log.write("FAIL")
test_thresholds.py
ct_min = 4.2
ct_max = 5.3
3 Answers 3
In-band error signalling
return signal_data
# ...
# reports false if message fails to be received
return False
is problematic. You're forcing the caller of this code to understand that the return value has at least two different types: boolean or whatever "signal data" is.
The Python way to approach this is to use exceptions. Rather than (say) re-throw AttributeError
, it would probably make more sense to throw your own exception type.
Also, the logic around retry counts is a little convoluted. You should be able to assume that if the loop has ended without returning, it has failed. Also, don't increment the counter yourself. In other words,
for attempt in range(5):
msg = bus.recv(1)
try:
if msg.arbitration_id == message.arbitration_id:
message_data = db.decode_message(msg.arbitration_id, msg.data)
signal_data = message_data.get(signal)
return signal_data
except AttributeError:
pass
raise CANBusInactiveError()
I would go a step further. My guess is that msg
- if it fails - does not have the arbitration_id
attribute. So - rather than attempting to catch AttributeError
- either:
- call
hasattr
, or - (preferably) call
isinstance
.
Context management
Put this:
verbose_log = open("verbose_log.txt", "a")
verbose_log.close()
in a with
.
Hard-coded paths
'C:\\Users\\tw\\Desktop\\dbc_file.dbc'
should - at least - go into a constant variable. Better would be to get it from a command-line argument, a conf file or an env var.
-
\$\begingroup\$ I have never used or seen simply try and finally(i.e., try without except). I read the docs and they said that if try gets an error the error is eventually raised after finally. Is try with finally actually useful anywhere? \$\endgroup\$Vishesh Mangla– Vishesh Mangla2020年06月28日 16:10:00 +00:00Commented Jun 28, 2020 at 16:10
-
\$\begingroup\$ @VisheshMangla yes. Most commonly when you need the effect of a context manager but one is not implemented. \$\endgroup\$Reinderien– Reinderien2020年06月28日 16:11:22 +00:00Commented Jun 28, 2020 at 16:11
-
\$\begingroup\$ Oh, I didn't know that context managers are coded inside. I used
with
statement blindly with any function that has.open()
and.close()
up till this point. This was when I knowwith
is nothing but try: except from the python docs. Thanks for the info @Reinderien. \$\endgroup\$Vishesh Mangla– Vishesh Mangla2020年06月28日 16:17:02 +00:00Commented Jun 28, 2020 at 16:17 -
\$\begingroup\$ @Reinderien thank you for your input. I've corrected the hard-code path to a constant variable. I've never set a path based on a command line argument, conf file or env var. \$\endgroup\$Tim51– Tim512020年06月29日 16:24:08 +00:00Commented Jun 29, 2020 at 16:24
-
\$\begingroup\$ @Reinderien I'm not sure how to use the context manager to mange the file when I need to "pass" the file object to other modules in my script. Each time I perform a test_case function in my
test_case.py
they will write to the file. So that's why I established the file without the context manager \$\endgroup\$Tim51– Tim512020年06月29日 16:26:05 +00:00Commented Jun 29, 2020 at 16:26
Protected Variables
Underscore is used to mark a variable protected
in python classes
_counter = 0
should be
counter = 0
Use of min_<foo<max_ is permitted in python
if (ct > ct_min) and (ct < ct_max):
can be
if ct_min < ct < ct_max:
-
\$\begingroup\$ I read this second one in some PEP few months ago. Can someone remind me which PEP was that? \$\endgroup\$Vishesh Mangla– Vishesh Mangla2020年06月28日 15:46:34 +00:00Commented Jun 28, 2020 at 15:46
-
\$\begingroup\$ I don't know that it's in a PEP, but: docs.python.org/3/reference/expressions.html#comparisons \$\endgroup\$Reinderien– Reinderien2020年06月28日 15:52:57 +00:00Commented Jun 28, 2020 at 15:52
-
\$\begingroup\$ Thanks for the help @Reinderien . \$\endgroup\$Vishesh Mangla– Vishesh Mangla2020年06月28日 15:55:38 +00:00Commented Jun 28, 2020 at 15:55
-
\$\begingroup\$ @VisheshMangla thank you. I have corrected
_counter
, I originally mistook the meaning for the_
. I've also corrected theif
statement condition as well. \$\endgroup\$Tim51– Tim512020年06月29日 16:27:49 +00:00Commented Jun 29, 2020 at 16:27 -
\$\begingroup\$ Nice work. See
Time for some Examples
here ->studytonight.com/python/access-modifier-python. \$\endgroup\$Vishesh Mangla– Vishesh Mangla2020年06月29日 16:30:22 +00:00Commented Jun 29, 2020 at 16:30
Not familiar with CAN yet so just one suggestion:
Get rid of those print
statements, use the logging module instead. See also here: Basic Logging Tutorial and here: Logging Cookbook
Some benefits:
- being able to write to multiple destinations: console + text files in different formats if desired
- increase or reduce verbosity at will
- keep a permanent record of program activity (and exceptions !)
It's easy to miss console output and not having a persistent, timestamped log makes it more difficult to track bugs or investigate incidents.
If your application is mission-critical/ unattended you could send logging output to a dedicated log collector, which could generate alerts when warning/error messages are emitted, or when your application crashes for some reason.
receive
function? It is not clear to me why that function needs to take a message to receive? \$\endgroup\$receive
function is used intc_1
. Its is supposed to take the message ID and the signal as args. Then return the value of the signal at that given instant \$\endgroup\$