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.

Sign up for GitHub

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

Jump to bottom

Doip improvements #435

Merged
merged 14 commits into from
Sep 26, 2023
Merged

Doip improvements #435

merged 14 commits into from
Sep 26, 2023

Conversation

ferdinandjarisch
Copy link
Contributor

Generic DoIP-related stuff.

See individual commits for changes...

@ferdinandjarisch
chore(doip-discover): Remove now obsolete try blocks around conn.close()
fd7e8dc 
In a2e9cee the conn.close() method was
fixed to no longer raise errors on closed connections.
@ferdinandjarisch
fix(doip-discover): Fine-tune logging output and print correct array
f4b2ff3
@ferdinandjarisch
fix(doip): Harden read worker against unknown packets
ac26227
@ferdinandjarisch
feat(doip): Introduce DoIP-specific connection errors
aa68d6e
@ferdinandjarisch
feat(doip-discover): Utilize newly implemented DoIP errors
79e67d8
@ferdinandjarisch
fix(doip): Catch TargetUnreachable NACK at .write instead
a5f40b7 
While changing no behavior for UDS this allows the DoIP discoverer to
deep-call into DoIP transports and handle TargetUnreachable by itself
@ferdinandjarisch
chore(doip): Log write message before calling write func
4847b13
@ferdinandjarisch
feat(doip-discover): Treat TargetUnreachable as expected error
f28e6cf
@ferdinandjarisch
fix(doip): Rework error-handling in _read_ack to be more logical
5719601
@ferdinandjarisch ferdinandjarisch removed the request for review from peckto September 22, 2023 12:07
@rumpelsepp
Copy link
Member

I looked through your changes, looks good.
Why did you remove ac26227#diff-f660a710f16526600ee2030ded239303921184cb7f83fe263bce71c5e843f132L107 this?
I copied the package definitions from the standard, so I figure it should be there even when it is currently unused.

@ferdinandjarisch
Copy link
Contributor Author

Why did you remove ac26227#diff-f660a710f16526600ee2030ded239303921184cb7f83fe263bce71c5e843f132L107 this?

While the "PayloadTypeSpecificMessageContent" is part of the DoIP packet, it is not part of the GenericHeader; it actually follows after the GenericHeader, as effective payload of the packet.

It does not make sense to have this field in the GenericHeader, and obviously it was never used anyhow.

@rumpelsepp
Copy link
Member

Ah. Ok.

@ferdinandjarisch
Copy link
Contributor Author

I will give this another test run on Monday and if successful will press merge.

Thanks a lot! :)

@rumpelsepp rumpelsepp mentioned this pull request Sep 22, 2023
Open
@ferdinandjarisch
Copy link
Contributor Author

@rumpelsepp

Tested and looks good to me.

I added another three commits on top; if those are fine with you, go ahead and merge!

@ferdinandjarisch ferdinandjarisch merged commit 91a9920 into master Sep 26, 2023
9 checks passed
@ferdinandjarisch ferdinandjarisch deleted the doip-improvements branch September 26, 2023 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rumpelsepp rumpelsepp rumpelsepp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ferdinandjarisch @rumpelsepp