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

Inconsistent Position Definition for Serverdown Tickle and PlayLoop #679

Open
vt128 opened this issue Sep 27, 2023 · 2 comments
Open

Inconsistent Position Definition for Serverdown Tickle and PlayLoop #679

vt128 opened this issue Sep 27, 2023 · 2 comments

Comments

@vt128
Copy link

vt128 commented Sep 27, 2023

Issue Description:
The Serverdown Tickle and PlayLoop commands in the firmware have inconsistent position definitions despite sharing the same definition according to HID instructions.

Reproducing the Issue:

Based on experiments conducted on a mk2 device, it has been observed that the start and end positions of the Serverdown Tickle command are handled differently compared to the PlayLoop command. The start position of the Serverdown Tickle command is inclusive, while the end position is exclusive. On the other hand, both the start and end positions of the PlayLoop command are inclusive.

Here are the observed results for different start and end positions:

Start Position End Position Play Loop Tickle
0 0 0-31 0-31
0 31 0-31 0-30
0 32 0-31 0-31
0 1 0-1 0 only

Diagnostic Investigation:
To better understand the cause of this inconsistency, the firmware code was carefully examined. The following code snippets are relevant to the Serverdown Tickle and PlayLoop modes:

  • Server mode tickle: link
  • PlayLoop mode: link
  • Main loop about playing: link

Based on my understanding of the code (correct me if I'm wrong ❤️), the following observations were made:

For Server mode tickle:

  • If the end position from the input is 0, it will be set to 1, resulting in a loop with only one element (position 0). However, the actual behavior is a loop from position 0 to 31.
  • If the end position from the input is 31, it will be set to 32, resulting in a loop from position 0 to 31. However, the actual behavior is a loop from position 0 to 30.
  • If the end position from the input is 32, it will be set to 32, resulting in a loop from position 0 to 31, which is the correct behavior.

For PlayLoop mode:

  • If the end position from the input is 0, it will be set to 32, resulting in a loop from position 0 to 31, which is the correct behavior.
  • If the end position from the input is 31, it will be set to 32, resulting in a loop from position 0 to 31, which is the correct behavior.
  • If the end position from the input is 32, it will be set to 33, resulting in a loop from position 0 to 32. This would have caused an out-of-bound error (maybe crash the device?) in code like ctmp = pattern[playpos].color;, even though the actual behavior is a loop from position 0 to 31 with no error.

Expected Outcome:

It is expected that the position definitions for Serverdown Tickle and PlayLoop commands align with the HID instructions and have consistent behavior. The observed inconsistencies should be resolved to ensure the expected behavior is maintained.

Thank you for taking the time to review this issue! It is hoped that this inconsistency can be addressed in future updates, providing clarity and resolving the confusion surrounding this matter 💯

@todbot
Copy link
Owner

todbot commented Sep 27, 2023

Hi! Thanks for the detailed report. Agreed that it does look like an error. I think this was not caught because most users color patterns are very short (2-8 lines).

Unfortunately blink(1) mk2 will not be receiving any further updates, as it's not easy to update the mk2 in the field. If you'd like to replace your mk2 with a mk3, please email us at blink1 at thingm.com and we can arrange a free (except for shipping) replacement.

I'll leave this issue open for others, in case it's an issue for them. I'll also take a look at the mk3 firmware to see if this issue exists there and perhaps issue a firmware update for it.

Thanks again!

@vt128
Copy link
Author

vt128 commented Sep 28, 2023

@todbot Thank you for your prompt response! I really appreciate your attention to the detailed report I provided. I just corrected a few typos in the original report --- in some scenarios, the blink(1) mk2 was expected to crash, but it did not. There's what makes me confused.

I will ensure compatibility for this in the higher-level software to prevent inconsistencies caused by the mk2 firmware.

I'm glad to hear that you are offering a replacement for the blink(1) mk2 with a mk3. However, I would like to keep my mk2 device because both my friends and I have the same model, and we have developed our projects based on them. In addition, if it is possible, I would be interested in obtaining other models such as the mk1 and mk3 for testing purposes, and I am willing to cover the shipping expenses for these free units, and having them would greatly aid in our development and testing efforts.

Once again, thank you for your understanding and assistance!

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

No branches or pull requests

2 participants