search

Openwide-Ingenierie / robotframework-can-uds-library

35 stars 14 forks source link

Hotfix/padding #4

Open Frankyboy100880 opened 4 years ago

Frankyboy100880 commented 4 years ago

Automatic padding with 0x55 for CAN Frames, so that UDS commands must not be filled with paddings to be eight bytes aligned.

RomainNaour commented 4 years ago

Hello,

Ok this allow to enable padding but can you explain why you use 0x55 ?

https://github.com/pylessard/python-can-isotp/blob/master/doc/source/isotp/implementation.rst https://github.com/pylessard/python-can-isotp/blob/5ab7f7e9843004197813cfec3b1ecadf6e5e4bc4/isotp/protocol.py#L673

Frankyboy100880 commented 4 years ago

Hello,

Ok this allow to enable padding but can you explain why you use 0x55 ?

https://github.com/pylessard/python-can-isotp/blob/master/doc/source/isotp/implementation.rst https://github.com/pylessard/python-can-isotp/blob/5ab7f7e9843004197813cfec3b1ecadf6e5e4bc4/isotp/protocol.py#L673

Because, I have to choose some padding. 0x00 and 0xff would also do the same? Should I switch to 0xff or 0x00? Actually, every value would do it.

RomainNaour commented 4 years ago

Hello, Ok this allow to enable padding but can you explain why you use 0x55 ? https://github.com/pylessard/python-can-isotp/blob/master/doc/source/isotp/implementation.rst https://github.com/pylessard/python-can-isotp/blob/5ab7f7e9843004197813cfec3b1ecadf6e5e4bc4/isotp/protocol.py#L673

Because, I have to choose some padding. 0x00 and 0xff would also do the same? Should I switch to 0xff or 0x00? Actually, every value would do it.

It seems 0xCC is used by default, providing a value to tx_padding allow to set "must_pad = True". Can you check if 0xCC work for you ? Otherwise, you change is ok.

Frankyboy100880 commented 4 years ago

Hello. I changed the padding to 0xCC as you suggested. We tried several values and there was really no effect, so that 0xCC should also work. I'll try tomorrow with real hardware.

RomainNaour commented 4 years ago

Hello. I changed the padding to 0xCC as you suggested. We tried several values and there was really no effect, so that 0xCC should also work. I'll try tomorrow with real hardware.

Thanks for testing, I'll wait for your feedback after your test with real hardware.

Frankyboy100880 commented 4 years ago

My colleague tried 0xCC option with real hardware and there was difference in the reply. IMHO the feature can be merged back to master. Many thanks for fast replies, we will see how far we can get with your library. :-)

RomainNaour commented 4 years ago

Hello @remdzi, Can you have a look? Thanks!

remdzi commented 4 years ago

Hi @RomainNaour , Hi @Frankyboy100880,

In automotive industry the usage of padding is variable for various car manufacturers. For (imaginary) examples:

Then it is necessary that the library becomes configurable to:

@Frankyboy100880 can you rework your pull request in order to implement these parameters?

@RomainNaour this will "break" the api of the library so this should be merged in a major release.

Regards Rémy