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

Add MemoryExtended services #1362

Merged
merged 6 commits into from
Dec 4, 2023
Merged

Conversation

RealByron
Copy link
Contributor

@RealByron RealByron commented Nov 27, 2023

Description

add MemoryExtended services : Read, ReadResponse, Write, WriteResponse

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code quality improvements to existing code or addition of tests

Checklist

  • The documentation has been adjusted accordingly
  • Tests have been added that prove the fix is effective or that the feature works
  • The changes are documented in the changelog (docs/changelog.md)

@RealByron
Copy link
Contributor Author

I had some problem with mypy, so I need some help

xknx/telegram/apci.py Show resolved Hide resolved
@RealByron
Copy link
Contributor Author

image
I don't know how to do that properly. I will dig on knx handbook, I probably miss something

Comment on lines 534 to 537
# inject [0x00] before 3 bytes address to enable unsigned int unpack
count, address, data = struct.unpack(
f"!BI{size}s", bytes([raw[2], 0x00]) + raw[3:]
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# inject [0x00] before 3 bytes address to enable unsigned int unpack
count, address, data = struct.unpack(
f"!BI{size}s", bytes([raw[2], 0x00]) + raw[3:]
)
count = raw[2]
address = int.from_bytes(raw[3:6], "big")
data = raw[6:]

Here is an alternative approach to this. I think it is easier readable since no 0-Byte injection is required.
Optionally, you could raw[6:size] too.

Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fully agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍 Can you do that for the other classes too and make CI pass? I think we would be fine to merge then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed them, fix now

@RealByron RealByron force-pushed the add_MemoryExtended_services branch from 237da02 to 7a42f20 Compare December 2, 2023 11:34
@farmio farmio changed the title add memory extended services Add MemoryExtended services Dec 3, 2023
Copy link

codecov bot commented Dec 3, 2023

Codecov Report

Merging #1362 (d5ef03b) into main (888df38) will increase coverage by 0.02%.
Report is 2 commits behind head on main.
The diff coverage is 99.09%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1362      +/-   ##
==========================================
+ Coverage   96.56%   96.59%   +0.02%     
==========================================
  Files         150      150              
  Lines        9584     9695     +111     
==========================================
+ Hits         9255     9365     +110     
- Misses        329      330       +1     
Files Coverage Δ
xknx/telegram/apci.py 98.93% <99.09%> (+0.02%) ⬆️

Copy link
Member

@farmio farmio left a comment

Choose a reason for hiding this comment

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

Thank you very much! 👍

@farmio farmio merged commit d7a22ab into XKNX:main Dec 4, 2023
11 checks passed
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