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

Fix MID 0002 revision 6 implementation #41

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ferm10n
Copy link
Contributor

@ferm10n ferm10n commented Mar 24, 2023

closes #40

@ferm10n ferm10n changed the title fix: missing keys in mid0002 parser Fix MID 0002 revision 6 implementation Mar 24, 2023
@ferm10n
Copy link
Contributor Author

ferm10n commented Mar 24, 2023

I discovered something potentially significant. This PR will cause this test to fail: Should handle error of generic communication with reply of request MID0004 Error ~ 74/76 - Based Test 04/07/2018

It appears that the sample data captured for the mid 0002 is missing the stationID, stationName, and clientID. I'm not sure how this data was captured, but it might make sense that it'd be missing these if node-open-protocol was used to capture the payload (since its parser for mid 0002 seems wrong).

Still, it's cause for concern, and makes me want to confirm what I think is a bug is actually a bug! But this is problematic since it appears this project has been unmaintained for a while now...

@ferm10n
Copy link
Contributor Author

ferm10n commented Mar 24, 2023

My earlier theory feels wrong, because the data length in the sessionControlClient test I mentioned isn't long enough to capture the missing keys. I assume that data was captured from a real tool, but that would mean that a power focus 4000 was not correctly implementing the mid. none of this seems to add up...

ferm10n pushed a commit to RV-Argonaut/node-open-protocol that referenced this pull request Mar 24, 2023
see the comments on this MR st-one-io#41
see the comments on this MR st-one-io#41
@ferm10n ferm10n force-pushed the 40-fix-mid0002-parser-missing-keys branch from db8f38b to 6c64d0f Compare March 28, 2023 17:56
@ferm10n
Copy link
Contributor Author

ferm10n commented Mar 28, 2023

I've modified the test to accept what I believe is a correct mid 0002 payload.

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.

MID 0002 revision 6 is incorrectly implemented
1 participant