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

Quick bug fix for Issue #5 #6

Merged
merged 2 commits into from
Apr 25, 2023
Merged

Quick bug fix for Issue #5 #6

merged 2 commits into from
Apr 25, 2023

Conversation

cbattlegear
Copy link
Contributor

This handles the bug in Issue #5 by making sure we always read 64 bytes of data.

After some research I think why this worked on Mac is because OSX looks like they just send the data while Windows pads the data to 64 bytes.

You'll want to test the build on your OSX box to see if this still works properly or if you'll need two different builds for windows/osx.

Also added Ox for all the hex debug outputs and fixed a potential bug if you hit the button during a read you could potentially over write the buffer. So now there is a Read buffer, Write Buffer, and Input buffer.

@spuder
Copy link
Owner

spuder commented Apr 23, 2023

Awesome work. Thank you so much for contributing this.

In my testing I'm finding unfortunately that the LED no longer works on Mac. (But does work on windows now).

Since the official Mute-Me can be moved between windows/mac seamlessly, I'd like to emulate that same behavior and avoid multiple OS specific builds.

Correction:
I am finding that eventually the LED does turn on, however it turns on red and won't change.

I'm guessing that the buffer is causing the issue.

            if(byte_count < 64) {
                rawhidBuffer[byte_count] = RawHID.read();
                byte_count++;
            } else {
            ```

@spuder
Copy link
Owner

spuder commented Apr 23, 2023

What's the thinking behind the input buffer? I assumed that the HID library was taking care of that (but it's been a while since I looked at the code, I may be wrong)

This feels like a pagesize issue and might be related to this NicoHood/HID#133 (comment)

@cbattlegear
Copy link
Contributor Author

Oh interesting that it now doesn't work on the Mac so in theory Mac is using a different buffer size for messages compared to Windows. Running the tracing I was seeing an essentially hard set 64b message size. Hence the 64 byte count buffer that is just getting read from every 64 bytes.

Going to try a different option where instead of hardcoding the amount I just do the report read after the bytesAvailable loop.

@cbattlegear
Copy link
Contributor Author

Some extra commentary here, still using the input buffer. Looked at the source of RawHid and it seems to use the buffer you give it in the "begin" method strictly for itself. So for output and for holding the data that has been sent in I also have separate 64B buffers.

The important parts in this build though are, I am using the bytesAvailable loop to pull in the report instead of hard coding to the 64B. The report is written to the buffer and then all processing is done after that.

I also added an extra debug output that just dumps the raw bytes of the currently read report. It should provide Raw bytes and number of bytes per report.

I don't currently instantiate per loop though, which may be needed. Either way, hopefully this works on Mac also, it should at least remove the hardcoded size per report which I think will fix both sides.

@spuder
Copy link
Owner

spuder commented Apr 24, 2023

Excellent work. I just tested on my mac and everything appears to be working perfectly. Thanks so much!

Is this ready to merge?

}
#ifdef DEBUG
for (byte i = 0; i < byte_count; i++)
Copy link
Owner

Choose a reason for hiding this comment

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

Minor linting issue here, tabs vs spaces?

@cbattlegear
Copy link
Contributor Author

This should be ready to merge as is, I didn't really run through any specific linting, did you have one in particular you have been using?

@spuder spuder merged commit 15798bb into spuder:main Apr 25, 2023
@spuder spuder mentioned this pull request Apr 25, 2023
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