Skip to content

Conversation

@mnbbrown
Copy link
Contributor

@mnbbrown mnbbrown commented Jul 5, 2024

ReadPropertyMultipleAck::new_from_buf doesn't take a reader, so it
will attempt to decode from the beginning of the slice. This fixes it to
only pass the remaining buf.
(you beat me too it)

Add more support for decoding ReadRangeValueType (bool, unsigned,
signed and NULL).

@mnbbrown mnbbrown force-pushed the fix-read-property-multiple-read-range-value branch 2 times, most recently from c54cc6d to 34a1947 Compare July 5, 2024 13:30
buf,
TagNumber::ContextSpecificOpening(Self::DATE_TIME_TAG),
"ReadRangeItem decode",
"ReadRangeItem decode open date time",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me know if I should back these changes out - but it made it much easier to debug.

Copy link
Owner

Choose a reason for hiding this comment

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

They are a little redundant because you have access to the Tag there but if they made your life easier then I have no problem at all putting them in. They are there for debugging after all.

@ninjasource
Copy link
Owner

ninjasource commented Jul 5, 2024

I don't think it's a good time to add anything right now or you're going to have some painful merge conflicts to resolve. I'm busy making significant changes to solve these ownership issues.

@ninjasource
Copy link
Owner

Ok the major refactor is done for now. I'm not 100% happy with it (ideally I would like to get rid of all the lifetimes if the alloc feature is enabled but that would involve writing another macro and I need to think about weather or not it makes things better or worse (more obscure).

Self {
function,
npdu,
raw_payload: &[],
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned in a comment in this PR, this should not be here as keeping track of segments is not the job of the codec. It should be done at a higher level.

@mnbbrown mnbbrown force-pushed the fix-read-property-multiple-read-range-value branch from b800b45 to 44edc59 Compare November 5, 2024 16:04
@ninjasource
Copy link
Owner

Thanks for your changes, they look good! My apologies for taking my time in getting back to this.

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.

2 participants