DID codec behavior for DIDs not in DidConfig

[lapo4719]

I want to read DTC snapshots without adding a DID codec to the DID codec dictionary in the userconfig's. I want to provide/define a default codec parsing behavior where it will just spit out the raw bytes to the response.service_data() when it can do nothing better, without it failing on me like shown in the error message:

Context: I am receiving DIDs that I don't have codec entries for (this is a problem I could avoid on my end and this all wouldn't be a problem, but I'm seeking to handle this situation with raw bytes data, rather than the exception that occurs.)

this is where I'm adding my current DID# to Codec dictionary, but I don't have DID entries for the DIDs I'm receiving.

This is where I'm reading DTC data, where the library will try to decode the received DIDs

File "C:\Git\diagnostics_tool\lib_diagnostic_tool\uds_utils\uds_driver.py", line 666, in request_read_dtc_snapshot_info dtc_snapshot_response: Response = self.uds_client.get_dtc_snapshot_by_dtc_number(dtc=dtc_id, record_number=snapshot_record_depth) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "c:\Git\diagnostics_tool\venv\Lib\site-packages\udsoncan\client.py", line 1413, in get_dtc_snapshot_by_dtc_number return self.read_dtc_information(services.ReadDTCInformation.Subfunction.reportDTCSnapshotRecordByDTCNumber, dtc=dtc, snapshot_record_number=record_number) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "c:\Git\diagnostics_tool\venv\Lib\site-packages\udsoncan\client.py", line 128, in decorated return func(self, *args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "c:\Git\diagnostics_tool\venv\Lib\site-packages\udsoncan\client.py", line 1626, in read_dtc_information raise error File "c:\Git\diagnostics_tool\venv\Lib\site-packages\udsoncan\client.py", line 1610, in read_dtc_information services.ReadDTCInformation.interpret_response(response, **response_params) File "c:\Git\diagnostics_tool\venv\Lib\site-packages\udsoncan\services\ReadDTCInformation.py", line 564, in interpret_response didconfig = ServiceHelper.check_did_config(did, didconfig) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "c:\Git\diagnostics_tool\venv\Lib\site-packages\udsoncan\services\__init__.py", line 129, in check_did_config raise ConfigError(did, msg='Actual data identifier configuration contains no definition for data identifier 0x%04x' % did) udsoncan.exceptions.ConfigError: Actual data identifier configuration contains no definition for data identifier 0xc422

And this is the error for not have a configuration for this DID.

Possible Solution There is way to add a feature for the user to control the default codec behavior on their end: I can use a defaultdict instead of a regular dictionary, and the user can add their own default DidCodec like below did_nums_to_codecs: defaultdict[int, DidCodec] = defaultdict(partial(DidCodec, 64))

print (did_nums_to_codecs['missing_codec']) result >> <udsoncan.DidCodec object at 0x000002A1FF553550>

but, the currently implemented way of checking that the did exists prevents this from working: ^ this if key in dict check won't kick-off the defaultdict's factory. If the check in the library is instead replaced with if not didconfig.get(did): then a default can be provided even for DIDs that aren't known.

Could this check be changed in the library?

Thank you greatly for your work

[pylessard]

I understand. Well explained and reasonable request. Expect this to be fixed sooner than later. Cheers

[pylessard]

Can you give a try to this branch? https://github.com/pylessard/python-udsoncan/tree/handle-default-did

[lapo4719]

Thank you for the response!

I made a small mistake, defaultdict does not trigger its .default_factory on a mydict.get(key) call, it triggers it only when we use the brackets:

The missing(key) dunder that defaultdict overrides has the same interface as value = mydictionary[key], which is that if it fails it throws a KeyError. Unlike .get(), which returns None by default or a user provided default.

Can you switch it to use the following: https://docs.python.org/3/library/collections.html#collections.defaultdict https://realpython.com/python-defaultdict/ ^ This does work! At least in creating this small behavior and creates a new dict entry (it went from 20 ->21)

[lapo4719]

Next Steps: But now I must decide how best to construct a default codec. it also appears that just using Partial(DidCodec, struct_string) means that I need one struct_string that always works. And struct.pack/unpack doesn't allow variable length, so lack of a known/dynamic length feels like a fundamental problem. But unless you can magically detect DID length at run time, I think this is my problem. lets keep going and see if it can partially work anyway:

using did_config = defaultdict(partial(DidCodec, '10s') # which should just be 10 characters, idk it might get chopped off

Sadly The .get_dtc_snapshot_by_dtc_number() call doesn't parse the service data as expected, and drops the data. (I wasn't seeing any of these did's fail to fill the service_data[0] before when I provide a valid DID definition.)

Digging into it, reading the data is fine, it has a reasonable payload:

But sometimes .service_data didn't get filled with the dtcs[]. I not sure why or how all this works.

The data gets properly parsed as a snapshot record and a dtc gets appended to dtcs[]:

On some of the get_dtc_snapshot_by_dtc_number() calls it doesn't fill the dtcs[] with anything, and it fails.

EDIT: I took more care and redid the screenshots:

This dtc data works and gets populated (library parsing of data) (user code reading it after)

This dtc data doesn't get populated and fails

[pylessard]

I can add a "default_codec" parameter if that helps. You can also implement a dict-like object that defines __contains__ and __getitem__ I think that will go through.

Let me know what you think

[lapo4719]

I think that might be better

[pylessard]

Which of the 2 options?

[lapo4719]

Sorry I misread. I think the first idea, a default_codec, would be good.

[pylessard]

Will have a look at this soon.

[lapo4719]

Is there a way to define a DidCodec that can upack a payload with arbitrary length or can figure out the length of the raw data payload from the lower layers? I'm not an expert at UDS services, and I'm not sure it allows for it, but that would be very nice behavior.

[pylessard]

The way UDS is designed, you need to know the length of the data in advance because you can read many DID in a single request and the length is never encoded.

There is a non-standard hack you can do to read the whole payload. It only works for DID that are the last one being read in a request. This feature has been requested by a user, but it's nothing standard. It can be achieved by throwing an exception in the len function.. not the cleanest implementation, but works for a hacky feature. see __len__ here : https://udsoncan.readthedocs.io/en/latest/udsoncan/helper_classes.html#udsoncan.DidCodec

[lapo4719]

@pylessard maybe don't support this idea.

I can think of a workaround: Right now I have a small list of Did codecs that I added manually, and then I built the ability to read a CDD file for the rest. My goal with this was to be able to provided a reduced ability to read DID data if a user doesn't provide a CDD file, or if the CDD file is missing a DID entry because of being out of date.

But instead of asking you to build a default mechanism, I can just build a table from 0x0000 to 0xFFFF and add my default in post for any DIDs that aren't assigned one yet. It's bulky, but you won't have to add anything.

I will pursue this, and you can choose if you'd like to support a default_codec to save space, but optionally

thank you very much for looking into this, and the DidCodec.ReadAllRemainingData suggestion

[pylessard]

ok You can also design your own extension of a dict that behave like you'd like. I consider cleaner than creating a config with 65535 entries

class MyDefaultDict(dict):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.default_val = None

    def set_default(self, v):
        self.default_val = v

    def __contains__(self, k):
        return super().__contains__(k) or self.default_val is not None

    def __getitem__(self, k):
        if not super().__contains__(k) and self.default_val is not None:
            return self.default_val
        return super().__getitem__(k)

Then

didconfig = {
    0x1234 : AsciiCodec(10),
    0x4567 : 'f'
}

didconfig  = MyDefaultDict(didconfig)
didconfig.set_default(MyDefaultCodec)

config['data_identifiers'] = didconfig

[lapo4719]

Tried this approach, it works to create the missing key: value pair, but this has unexpected consequences: The if 'data_identifiers' in didconfig causes that entry to be registered with the default value (a codec, not a dictionary) and then line 179 re-assigns the whole dictionary.

[pylessard]

oops, indeed! Guess the support of the new param will be required to get a clean solution.

[lapo4719]

Well I'm glad we tried everything. I'll try the generated 0x0000 to 0xFFFF table with default codecs as work around until you have time to look into the new param.

[lapo4719]

Regarding the ReadAllRemainingData trick. This does not appear to work for the DTCInformation interpret_response() specifically, because it needs to iterate over the remaining length, and doesn't yet handle this exception.

I am going to try a static length. It seems reasonable that doing this for the DTC snapshots is not as easy or useful as for other services because of maybe multi-did payloads.

[pylessard]

I implemented the default codec config. Want to try this branch?

support-default-did-codec

[pylessard]

I have merged the branch. reopen this issue if that doesn't fix your issue. Fixed in #194

[lapo4719]

Thank you so much for taking to time to add this. I'll be happy to test this out and confirm. but I'm unable to till after Christmas.

[JensLukaschkowitz]

Dear @pylessard,

in your merge request #194 you added a deep copy of the complete config structure in your check_did_config function in dids.py. This function is implicitly called twice while using Client.read_data_by_identifier by the make_request and interpret_response methods. So say a user user uses this read_data_by_identifier function per DID and has 20 DID, this will result in 40 deep copy operations, or?

And if the request is integrated in a time-based loop, so per time step 2*n, with n is the number of DIDs, deepcopy operations will occur. For big datapoint dictionaries this is from my point of view very time consuming.

Is there a way to reduce the number of deep copy operations to one or to only make a deep copy for the default case? In my case by commenting the deepcopy line I was able to reduce the time per DID read from 2,5s to 0,15s on a PI Zero.

Best regards Jens

[pylessard]

Interesting, did not consider the performance hit caused by this. will review soon

[pylessard]

@JensLukaschkowitz
Try this branch : https://github.com/pylessard/python-udsoncan/tree/fix-performance-degradation-caused-by-deepcopy-of-didconfig

Any better?

[JensLukaschkowitz]

@pylessard works great for me, thx for this quick fix

[pylessard]

Fixed in #205