Skip to content

diag-kernel: thread base offset through structure DOP decoding#282

Open
lorinciubeica10 wants to merge 2 commits intoeclipse-opensovd:mainfrom
lorinciubeica10:bugfix/structure-dop-base-offset
Open

diag-kernel: thread base offset through structure DOP decoding#282
lorinciubeica10 wants to merge 2 commits intoeclipse-opensovd:mainfrom
lorinciubeica10:bugfix/structure-dop-base-offset

Conversation

@lorinciubeica10
Copy link
Copy Markdown
Contributor

@lorinciubeica10 lorinciubeica10 commented Apr 15, 2026

Summary

Replaced push_slice/pop_slice coordinate shifting in map_structure_dop_from_uds with a base offset parameter propagated through all decode functions.
This correctly handles both absolute and relative byte position conventions without corrupting the payload cursor for inner params

Checklist

  • I have tested my changes locally
  • I have added or updated documentation
  • I have linked related issues or discussions
  • I have added or updated tests

Related

Fixes #270

Notes for Reviewers

@lorinciubeica10 lorinciubeica10 requested a review from a team as a code owner April 15, 2026 11:09
@lorinciubeica10 lorinciubeica10 force-pushed the bugfix/structure-dop-base-offset branch from 0825d20 to 4ca501c Compare April 15, 2026 12:02
@lorinciubeica10
Copy link
Copy Markdown
Contributor Author

Cargo.lock needed because of build_and_test failure:
image

@theswiftfox
Copy link
Copy Markdown
Contributor

Cargo.lock needed because of build_and_test failure: image

Thanks! :)
I created #284 a minute ago, as I stumbled across the same issue in my PR ^^
Feel free to 👍 or 👎 on the issue if you have any thoughts about it :)

@andreirusu-aumovio
Copy link
Copy Markdown
Contributor

Thanks @lorinciubeica10 ! I can confirm that this PR solves one of the issues I encountered while extending ODX for DTC Snapshot/Extended data where the response would be wrongly interpreted:
59 04 01 is actually the Positive Service response + Subfuction + First Byte of DTC instead of the actual DTC of 01 E2 40
image

Comment thread cda-core/src/diag_kernel/ecumanager.rs Outdated
Ok(())
}

#[allow(clippy::too_many_arguments)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Do you think it would make sense to introduce a struct that wraps the &datatypes::Parameter together with the base_offset?
Something like this:

struct ParamContext<'a> {
    parameter: &'a datatypes::Parameter,
    base_offset: usize,
}

My train of thought being that these two are related to each other, and combining them into a single parameter would avoid the clippy warning at the same time and maybe we have additional context we want to pass with the parameter in the future.

This is more of a discussion point, where I'd like your opinion on it as well. If you prefer it as is that is perfectly fine to me as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it s a good idea since base_offset is always passed alongside param. I will fix it

@lorinciubeica10
Copy link
Copy Markdown
Contributor Author

Added several fixes for bugs caused by incorrect or missing data when decoding UDS responses:

  • Parameters inside Structure DOPs were decoded using relative byte_position
    instead of absolute position in the full response payload.
  • map_dynamic_length_field_from_uds computed the wrong item count when a param
    had no explicit byte_position.
  • EnvDataDesc decoding used an incorrect base_offset, causing the discriminator
    lookup to fail and snapshot data to be silently dropped.
  • Extended data and snapshot requests always tried both FaultMem and UserMem
    service types; they now select the correct one based on the scope parameter.

@lorinciubeica10 lorinciubeica10 force-pushed the bugfix/structure-dop-base-offset branch 3 times, most recently from 561c13e to 31c7313 Compare April 21, 2026 09:14
@lorinciubeica10 lorinciubeica10 force-pushed the bugfix/structure-dop-base-offset branch from 31c7313 to cf4b74b Compare April 21, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Parsing issue for read data

3 participants