Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated readme #291

Merged
merged 5 commits into from
Oct 29, 2024
Merged

Updated readme #291

merged 5 commits into from
Oct 29, 2024

Conversation

mdabrowski1990
Copy link
Owner

Description

  • update other packages description (be more precise)
  • add a few details about py-uds

How Has This Been Tested?

  • just review

Process

I know the process and did my best to follow it

Update, provide more details
Be more precise, add more detailed information about udsoncan.
@mdabrowski1990 mdabrowski1990 added the documentation Improvements or additions to documentation label Oct 28, 2024
@mdabrowski1990 mdabrowski1990 self-assigned this Oct 28, 2024
@mdabrowski1990 mdabrowski1990 linked an issue Oct 28, 2024 that may be closed by this pull request
@mdabrowski1990
Copy link
Owner Author

@pylessard I would appreciate if you could review these changes.

Copy link

codecov bot commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a3cf879) to head (b805919).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #291   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           42        42           
  Lines         2743      2743           
  Branches       344       344           
=========================================
  Hits          2743      2743           
Flag Coverage Δ
integration-tests 80.96% <ø> (ø)
integration-tests-branch 75.79% <ø> (ø)
unit-tests 100.00% <ø> (ø)
unit-tests-branch 100.00% <ø> (ø)

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@pylessard
Copy link

Hey, I put some comments.
Most of your comments revolves around ISOTP stuff that are simply not the responsability of the UDS layer. I made my project 100% decoupled from the underneath layer while I see that your project also includes ISOTP. Each approach has pros and cons, but it is wrong to assume that it is "missing".

Modularity is always a bit more complex and harder to optimize. That could be the differentiator you put ahead : a turn-key solution that covers everything from application to hardware layer.

@pylessard
Copy link

Also, if you want to do something better than my repo, I suggest designing right from the start the possibility to have async calls with the client. My repo uses a stateful communication that can only accomodate a request/response approach. It's simple, but it has limitations. The nicer way would be to have a service thread that manages the communication. When a user "sends" a request, he is given a future object that can be awaited now or later. Having a background task would also allow to register callbacks on certain broadcast messages, like ResponseOnEvent

@mdabrowski1990
Copy link
Owner Author

Also, if you want to do something better than my repo, I suggest designing right from the start the possibility to have async calls with the client. My repo uses a stateful communication that can only accomodate a request/response approach. It's simple, but it has limitations. The nicer way would be to have a service thread that manages the communication. When a user "sends" a request, he is given a future object that can be awaited now or later. Having a background task would also allow to register callbacks on certain broadcast messages, like ResponseOnEvent

Yeah, exactly that is the plan to have an option to do stuff asynchronously. I have not prepared Client/Server/Sniffer classes yet to handle these top layers (in Model OSI, it would be layer 5 - Session).
I tried to document idea here (https://uds.readthedocs.io/en/stable/pages/knowledge_base/osi_model.html) in case you are interested. So layers 1/2 are hopefully handled by other packages (python-can, CANlib and scapy would definately be supported, so far only python-can has been integrated), layers 3/4 would be part of this package (as you mention it has its pros and cons, but I decided that would be easier to integrate everything this way. Later on I might add option to limit your installation (for example only UDS, UDS + DoCAN or UDS + DoIP) to tackle some cons.

@mdabrowski1990
Copy link
Owner Author

@pylessard I would appreciate if you referred to knowledge base (it is documentation with explanation of how UDS works) in your project(s) and took part in reviewing whenever a new part is released. No pressure of course ;)

Link: https://uds.readthedocs.io/en/stable/pages/knowledge_base.html

@pylessard
Copy link

Thanks,
I agree more with what I read now :)
I can add the link without issue.

One last thing, just like this conversation, the knowledge base mixes UDS and ISOTP like they were the same thing. There is no such thing as a UDS packet. Segmentation is defined by ISOTP (iso-15765), not UDS. That is very misleading. There are UDS messages that can be sent over an ISOTP frame. ISOTP frames are segmented in many PDU. A PDU is expected to be a CAN message.

I see you are putting lot of effort to explain the protocol. That's good, but make sure to refer to the standard and be exact

@mdabrowski1990
Copy link
Owner Author

Thanks, I agree more with what I read now :) I can add the link without issue.

One last thing, just like this conversation, the knowledge base mixes UDS and ISOTP like they were the same thing. There is no such thing as a UDS packet. Segmentation is defined by ISOTP (iso-15765), not UDS. That is very misleading. There are UDS messages that can be sent over an ISOTP frame. ISOTP frames are segmented in many PDU. A PDU is expected to be a CAN message.

I see you are putting lot of effort to explain the protocol. That's good, but make sure to refer to the standard and be exact

Yeah, that is true. Sometimes I was trying to hard to find a meaningful name. With PDU (or actually N_PDU, cause each OSI model layer has its own PDUs) it was not that easy ;)
You are right thought, I should stick to naming used in standard and UDS Packet -> N_PDU or just Packet.

@mdabrowski1990 mdabrowski1990 merged commit 8a42282 into main Oct 29, 2024
61 checks passed
@mdabrowski1990 mdabrowski1990 deleted the updated-readme branch October 29, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readme is misleading about udsoncan
2 participants