richclubb / python-uds Goto Github PK

In Uds.py:

If a request is sent with suppressRespose=True, the response is still tested for "response pending" (0x78), even though response is None. See code starting at line 140.

The response pending would only work for a single response pending too. There could be several.

Perhaps replace with this:

    if responseRequired:
          while True:
              response = self.tp.recv(self.__P2_CAN_Client)
              if not ((response[0] == 0x7F) and (response[2] == 0x78)):
                  break

Currently the project depends on the python-can package. This is a tight-coupling and needs to be addressed so that other CAN implementations can be used in future

A logger is needed to make sure that the transmissions are properly recorded to identify bugs.

CanTp.py

• for the condition on line 125 to make sense after line 121, presumably one of the above flows can update the state to END_OF_BLOCK - seems likely but just checking, and it may be worth a comment? ... if you can add one at line 70, then you can add one here

• other than usual comments, it otherwise looks good.

In UdsConfigTool.py, in "def readDataByIdentifier(self,diagnosticIdentifier)" ...
use of "self" like this in a non-class function can be a little confusing
... but I appreciate this is in the "experiments"
It's also light on comments describe what is happening here, but I know what it's doing. "target" may be a better parameter name than "self" in this context?

CanTpMessage.py

• no comment against the class, but looks like one was intended - nor against the constructor; worth a comment about the payload parameter?

• a little pedantic, but you mix your descriptions between payload and message (e.g. length was taken from payload length, but comments says returning message length). Not a problem in this context, but could be confusing in a larger more complex bit of coding. [same in other files - again, clear enough in most case, but not all, e.g. at least once the "message" is the overall CAN message, and the "payload" is payload extracted from it, so they are potentially different - so worth being careful!]

• line 56 " #if" ... is not a terribly useful comment

• "process state()" ... all paths through define "newState", so this is not a problem here, but normally worth a defensive default in case it drops through without (e.g. there could have been a coding error on a path that hadn't been checked)

• otherwise looks OK

Implementation of the tester present service and creation of container and methodFactory classes

This might be tricky as this may require some changes to the Uds communications component as this might need some kind of thread and semaphore to work correctly.

Adding support for ELM327 modules

Uds.py

• light on comments, but straightforwardly written (again this is a pretty general comment throughout the modules)

Just started - work in progress.

Currently the implementation is tightly coupled to the use of CAN and Can Transport Protocol. This should be abstracted out so that other transport protocols can be supported. for example: DoIP, LIN etc

UdsMessage.py

• no comment against the class, but it looks like one was intended

• comments run out from "response()" onward, although fairly obvious, and either trivial or not implemented yet

Currently it may be difficult to debug the function definitions as they are essentially hidden from the user.

A logging system is needed so that the actual function definition can be retrieved

Currently CanTp does not support extended addressing, this needs to be added in future

Vector have an auto-complete function in vFlash, this would be quite nice to have for an interactive IDE such as pycharm or similar.

ReadDataByIdentifierMethodFactory.py

• lacking any commenting

• you're dynamically creating new functions (in all methods) without ever checking if they've already been created - is this OK? i.e. do you want to allow an overwrite of an existing function, or are you preventing multiple creations elsewhere? Not a problem, but a little inefficient.

• otherwise it all looks nicely structured and can't see anything obviously wrong.

ReadDataByIdentifierContainer.py

• lacking any commenting

• you don't need a main section at the bottom unless there's code you want executed in the module if it is ever called directly - seeing as you only "pass" then you don't need it at all (same in some of the other files)

• again, in the static method, "self" isn't usually used in this context, so it will confuse some people - again "target" may be a better parameter name.

... by the way, are you aware of class methods? (@classmethod)

Basic code has been written. The current test ODX file is only being parsed sufficiently to ensure that the service is present. Beyond that, the format of the message is fixed in the spec and processed accordingly. This may need to be changed if Richard would prefer it done differently, but it works for now.

Currently includes the basic positive test cases to cover hard reset with or without response suppression, and positive testing of the negative response cases. Negative test cases (e.g. handling of an invalid negative response), has not been added yet (currently considered low priority).

Adding support for the LIN transport protocol

Create the method factory for creating the Read Data By Identifier Method

setup.py still has version at 0.1? (xml files show project version 4) [same throughout all code]

No headers or anything on your timer utilities

• again, other than usual comments, otherwise looks good.

Classes added to parse ihex files (ExtendedSegmentAddress, StartSegmentAddress, and StartLinearAddress record handling still needs to be added, but these were record types not present in any of the available ihex test files).

The classes expose all details at the primitive level for customised handling of transfers.

The classes also provide wrappers for simpler transfers.

Use of the wrappers has also been embedded within the Uds object to support attachment of an ihex file and simple transfer calls.

The Uds file has had a final wrapper added to support transfer of a specified ihex file with a simple reqDownload, transData and transExit combination.

Most of the options have been tested to some extent or other either informally or via the transData unit tests.

Merged to master after review with Richard.

UDS should get session timing from the server following a session change. This enables the client to manage timeouts better - most importantly the P2star used by "request received correctly, response pending".

This relates to #48

Work in progress

per ISO 15765-3, the P2_CAN_Client is the time between below two events:

• client (tester) sent out the diagnostic request

• client receives the first frame (or single frame) response from the sever (ECU/Vehicle).

Current implementation seems to be the between tester sent out diagnostic request and ECU sent out all the responses (first frame and all the consecutive frames).

This is causing the module raise timeout error when ECU is responding with multiple frames for one diagnostic request.

A suggestion was made that it would be good to have a link from the standard ISO-14229 defined DIDs to the ODX defined DIDs. This way a person can use the ISO naming convention but still get access to the parameters

Currently CanTp has a statically defined timeout for the timeoutTimer parameter. This should be configurable using the config file or the kwargs passing

DecodeFunctions.py

• specified pretty much everything in the documentation section at the top, except a version - do you need to at this level?

• the commenting runs out a bit early

• is there a reason for ...

def stringToIntList(aString, encodingType):
result = []
[result.append(ord(i)) for i in aString]
return result

... rather than ..

def stringToIntList(aString, encodingType):
return [ord(i) for i in aString]

... or ...

def stringToIntList(aString, encodingType):
result = [ord(i) for i in aString]
return result
... i.e. in the event that you might want to use the intermediate result before returning?

• intArrayToIntArray fails if given an unrecognised outputType - it's OK for inputType as you have the unconditional else - could just slap an unconditional else to trap it as for inputType, or possibly add a defensive exception trap around the code or allow the function simply throw an exception to the parent - if you follow the inputType structure, I would guess the "int8" path is the default?

General (occurs in both the examples and non-example code) - I'd have been tempted to have defined some of the values as constant, to make the code a little more readable - the code could then use the defined constants - you know this stuff backwards, so you don't need it so much, so it's easily overlooked. You have assigned them to variables in some cases, but it's embedded within the code, e.g. "positiveResponse = 0x62"

Also in the past, we used to follow a convention of using all caps with underscores for constants - I don't know if people still use that these days?

Same with things like "3" & "4" (num modules), and "4" & "28" (boot s/w id) used as payload indexing to extract number of modules - not the worse magic numbers ever, but worth 5 secs thought?

... there is some good enum use though - ODB.py, and ISO14229.py enums are good, so good look to those for examples for the rest

TpFactory.py

• does the class only ever have a single static method? (other than what's inherited from object)

• line 48: calling a static method "create" which doesn't appear anywhere? - a mistake or something in python I've not seen before? Looks like it should be calling "tpFactory('CAN')"?

• otherwise looks OK

The ISO-14229 standard defines that the RDBI service should support querying multiple DIDs. This is a valid enhancement to the RDBI method

UdsConfigTool.py (real one)

• no comments in the file (this is a common problem in other file as well)

• currTag (line 39) is not used anywhere

• there's an unnecessary "pass"at the bottom (it doesn't hurt, but it could be tidied up)

Implementation of the ECU reset service and its container and method factory classes.

CanTpListener.py

• line 16: "may want to be" ... looks like it now is

• line 32: an alternative would be to undefine "on_message_received()", e.g. " del self.on_message_received" - in practise both give a TypeError, but you'll get a different error message - the code is fine, but just in case you weren't aware of how to undefine things (the latter releases resources, whereas the former will hang on to the "None" and stuff. ... assuming of course that I've remembered correctly!

• otherwise looks OK

Basic code has been written, but not tested do to the service not being supported in the current test ODX file. Regression testing of request download and transfer data have been completed to reduce risk that the inclusion of this code has broken anything.

Although not runnable witht hte current ODX file, it currently includes the basic positive test cases to cover upload, and positive testing of the negative response cases. Negative test cases (e.g. handling of an invalid negative response), has not been added yet (currently considered low priority).

Note: this is almost identical to the Request Download service code.

Hello,
This is not an issue. I am looking for your help.
I am trying to get diagnostic data from ECU. I have designed a panel using the PyQt5 framework.
I've tried a few examples but nothing works out, it always throughs some exceptions.
I am using VN1630 as a CAN Interface. This is the panel

I tried using the raw send command example code

`from uds import Uds

ECU_Serial_no = Uds(transportProtocol="can", reqId=0x7E0, resId=0x7E8)
a = ECU_Serial_no.send([0x22, 0xF1, 0x8C])`

Traceback (most recent call last):
File "C:/Aasai/Python CAN Project/python-uds-master/python-uds-master/uds/Uds_CANoe_Test.py", line 19, in
ECU_Serial_no = Uds(transportProtocol="can", reqId=0x7E0, resId=0x7E8)
File "C:\Users\aasaialangaram\AppData\Local\Programs\Python\Python37-32\lib\site-packages\uds\uds_communications\Uds\Uds.py", line 43, in init
self.tp = tpFactory(self.__transportProtocol, configPath=configPath, **kwargs)
File "C:\Users\aasaialangaram\AppData\Local\Programs\Python\Python37-32\lib\site-packages\uds\uds_communications\TransportProtocols\TpFactory.py", line 49, in call
raise Exception("Unknown transport type selected")
Exception: Unknown transport type selected

It shows the Unknown transport type selected. Like that what are reqId and resId?

Could you help me to get the first response and solve this issue?

Thanks,
Aasai

Implementation of the write data by identifier service and its container and method factory classes

CanTpType.py

• again this is clear enough without comments, but worth any comments anyway? (a good habit if nothing else)

Implementing the session control class

Task to implement the Session Control service, container and method factory classes

Setup and begin the read-the-docs page for python-uds

Setup python-uds for installation with pip

Basic code has been written and tested.

Currently includes the basic positive test cases to cover data transfer, and positive testing of the negative response cases. Negative test cases (e.g. handling of an invalid negative response), has not been added yet (currently considered low priority).

Note: this is mostly hardcoded as for Request Download.

hi all,
I'm using the lib with vn1610 vector hardware , during a scan of service 22 xx yy I got this exception:
self.__recvBuffer.append(msg.data[self.__pduStartIndex:])
AttributeError: 'CanTp' object has no attribute '_CanTp__recvBuffer'
in file ....python_uds-1.0.2-py3.7.egg\uds\uds_communications\TransportProtocols\Can\CanTp.py

My script continue to run, but slowly: it sends 1 msg per second and not every 10 msec

Basic code has been written. The start SBL handling may have an issue - the test case is still in development, so not sure at present, so still classifying this as a work in progress.

Currently includes the basic positive test cases to cover start, stop and result cases with or without response suppression, and positive testing of the negative response cases. Negative test cases (e.g. handling of an invalid negative response), has not been added yet (currently considered low priority). A range of routines have been covered, based on availability in the test ODX file.

Work in progress.

Currently the configuration tool works for reading the default config but does not work for editing the parameters, this needs improvement

Basic code has been written and tested.

Currently includes the basic positive test cases to cover transfer exit, and positive testing of the negative response cases. Negative test cases (e.g. handling of an invalid negative response), has not been added yet (currently considered low priority).

Note: this is mostly hardcoded as for Request Download.