pylessard / python-udsoncan

Python implementation of UDS (ISO-14229) standard.
MIT License
586 stars 202 forks source link

Interpret data response length dynamicaly for one DID read #45

Closed wanam closed 4 years ago

wanam commented 4 years ago

This is to support dynamic codec length on the case of a Read Data Identifier request that contains only one DID.

pylessard commented 4 years ago

Hi, thanks for your help.

Does this change is justified by ISO-14229 or is this something you would simply find convenient for your application?

Also, any change of this size should be backed by unit tests. You probably make them fail now.

wanam commented 4 years ago

Hi @pylessard,

Actually we need this change for 2 reasons:

I didn't check the tests, this change should not impact rdbi with multiple dids, it should fail only on a test with single did read with no codec length specified, which I guess is not implemented.

pylessard commented 4 years ago

I see. So this behaviour isn't described by iso-14229 right? If that is the case, I would maybe do it a little differently. My concern is that the NotImplementedError is the default behaviour and I'd like the default behaviour to be compliant with the standard.

Regarding the tests : it is never a good practice to not test something because you believe it works. That's the whole point of making tests ;)

I will check soon and most likely propose a similar fix, but slighlty different.

Cheers

wanam commented 4 years ago

Understood :) , I agree that all relevant cases should be covered by tests, I was just talking about the existing tests I might break.

wanam commented 4 years ago

And I don't think this behavior is described in ISO-14229, I need to double check.

wanam commented 4 years ago

I checked the ISO-14229, section 10.2 ReadDataIdentifier (0x22) service.

And I'm surprised the response message flow does not specify anything about managing DID values by offsets in the response, so I guess you added the codec length requirement because the DID cannot be used as a trustful separator to split the response, am I right?

As expected the "TestReadDataByIdentifier" tests are still passing, but I think at least 2 tests need to be added if you agree to add dynamic length support for single DID requests:

pylessard commented 4 years ago

Correct, when I read the standard (2006 version), it was clear that one must knew the size of each DID to parse the response. There is no length encoded anywhere in the RDBI service response.

I'm not against adding the feature, but keep in mind your application may not be "standard".

Thanks for reviewing

pylessard commented 4 years ago

I've merged your changes but made a slight modification. You need to define a length that raise a special exception

def __len__(self):
    raise self.ReadAllRemainingData

I've done the unit tests. Let me know what you think. I will update the documentation accordingly afterward.

wanam commented 4 years ago

@pylessard Well done! Tested and working fine.

pylessard commented 4 years ago

Published under v1.10. Cheers