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

Firmware read pull io expander input #650

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Firmware read pull io expander input #650

wants to merge 1 commit into from

Conversation

purdeaandrei
Copy link
Contributor

No description provided.

@purdeaandrei purdeaandrei requested a review from whitequark as a code owner July 28, 2024 17:03
@purdeaandrei
Copy link
Contributor Author

-59 bytes optimization + 72 bytes for the feature, results in +13 bytes total change

@purdeaandrei
Copy link
Contributor Author

-59 bytes optimization
+72 bytes for the feature
-3 bytes eliminating redundant write
-32 bytes opimizing by using bit access instructions when accessing IO registers
results in -22 bytes total change

@purdeaandrei purdeaandrei force-pushed the firmware_read_pull_io_expander_input branch from f83d269 to c3f191b Compare July 28, 2024 19:59
@whitequark
Copy link
Member

There are many unrelated changes making this PR difficult to review.

@purdeaandrei purdeaandrei marked this pull request as draft August 1, 2024 00:45
@purdeaandrei
Copy link
Contributor Author

I've set this as a draft for now,
and I have split off two of the optimizations as separate PRs to make it easy to review:
#652
#651

I will clean this PR up once those PRs are resolved.

@purdeaandrei purdeaandrei force-pushed the firmware_read_pull_io_expander_input branch from 772fcd8 to ed5a255 Compare August 1, 2024 17:14
whitequark
whitequark reviewed Aug 1, 2024
View reviewed changes
Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

Thanks. The code in the PR is fine, however, I think it would be a better fit to our USB protocol to have a dedicated command to read the state of the I/O, as the USB protocol is designed in a way that prioritizes semantics over code size.

How about USB_REQ_IO with IN-only request or something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whitequark whitequark whitequark left review comments

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
@purdeaandrei @whitequark