Skip to content

DEV-216 clear CRC bytes for instream commands#212

Merged
JongChern merged 5 commits intomasterfrom
DEV-216
Apr 18, 2025
Merged

DEV-216 clear CRC bytes for instream commands#212
JongChern merged 5 commits intomasterfrom
DEV-216

Conversation

@marknolan
Copy link
Copy Markdown
Member

No description provided.

@marknolan marknolan requested review from JongChern and Copilot April 14, 2025 09:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java:1092

  • [nitpick] Consider renaming the 'clearCrcBytes' parameter to something more descriptive, such as 'shouldClearCrcFromBuffer', to clarify its purpose.
protected void processInstreamResponse(boolean clearCrcBytes) {

@marknolan
Copy link
Copy Markdown
Member Author

Just to note, a better approach would be to actually check the CRC in the instream response parsing:
https://github.com/ShimmerEngineering/Shimmer-Java-Android-API/blob/5713c85282388263c12d30c7e46a9cbdb2ee6d45/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java#L1152

And in the other response parsing:
https://github.com/ShimmerEngineering/Shimmer-Java-Android-API/blob/5713c85282388263c12d30c7e46a9cbdb2ee6d45/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java#L1861

but this would involve a much larger update as there are some command that don't have a fixed number of bytes in the response so they would all have to be managed individually while also having to manage what happens if/when the CRC check fails.

Copy link
Copy Markdown
Collaborator

@JongChern JongChern left a comment

Choose a reason for hiding this comment

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

ok looks good and tested

@JongChern JongChern merged commit 917e83e into master Apr 18, 2025
1 check passed
@JongChern JongChern deleted the DEV-216 branch April 18, 2025 07:09
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.

3 participants