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

Refactor debugging suggestions #35

Open
AndunHH opened this issue Jun 27, 2024 · 3 comments
Open

Refactor debugging suggestions #35

AndunHH opened this issue Jun 27, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@AndunHH
Copy link
Owner

AndunHH commented Jun 27, 2024

In #33 @simonmacarthur came with some good suggestion for refactoring the debug output. This issue is forked, because the original issue was closed successfully.

Suggestion 1:

Maybe have send_message return a bool to indicate if a message was sent?

-> Yes, in fact a good idea!

Suggestion 2:

There is no reason for it to be placed near the code that does the calculation as the flow will always run through to the end of the function (and in fact always call send_message) so it could be moved into a single call that takes all possible "watched" values and ...

-> We need to rewrite the good a little bit, because the centered array is mapped to the min-max values between debug=2 and debug=3. No big deal with an additional array. In every case, we must leave proper comments for new-comers in the code how to call the debug-reporting for this spots.

Suggestion 3:

and by changing the debug_level to a bitmask, would give better flexibility in determining which items people want output. (I'm happy to put something together by way of example if that helps).

-> Yes, also a good idea as an additional debuging. I think there is nothing simpler for newcomers than giving them numbers rising from 1 to 9 to perform ;) (and also typing this numbers of the serial interface is easier than a bitmask ;))
But we can obviously put a mapping from those numbers to general applicable bitmask.

@AndunHH AndunHH added the enhancement New feature or request label Jun 27, 2024
@AndunHH
Copy link
Owner Author

AndunHH commented Jun 27, 2024

I used the chance to integrate Suggestion 1 into #34.

@simonmacarthur
Copy link

Regarding 1:
Great 👍

Regarding 2:
Good spot, I forgot the mutation of the centered array values.

Regarding 3:

My suggestion would be:

#define DEBUG_RAW_VALUES 1
#define DEBUG_CENTERD_VALUES 2
#define DEBUG_VELOCITIES 4
#define DEBUG_KEYS 8
#define DEBUG_HID_MESSAGE 16
...

DEBUG_OUTPUT = DEBUG_RAW_VALUES + DEBUG_CENTERD_VALUES + DEBUG_VELOCITIES + ...

My view is this is easier to read for newbies ( but then that's just preference I guess ;) )

However we could also combine them into "friendlier" messages;

#define DEBUG_LEVEL_1 DEBUG_RAW_VALUES + DEBUG_CENTERD_VALUES
#define DEBUG_LEVEL_2 DEBUG_CENTERD_VALUES + DEBUG_KEYS_PRESSED 4

DEBUG_OUTPUT = DEBUG_LEVEL_1

etc.

(Apologies if I'm teaching grandmother to suck eggs ;) - again it's a style / preference thing)

@AndunHH
Copy link
Owner Author

AndunHH commented Jun 30, 2024

I like the idea! With the bit mask and the debug levels combined we get good and newcomer friendly good!

Do you write a pull request? I can add it to my list in the next week or so...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants