AndunHH / spacemouse

Repository for the space mouse based on four joysticks and keys
Other
39 stars 9 forks source link

Refactor debugging suggestions #35

Open AndunHH opened 5 days ago

AndunHH commented 5 days ago

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 commented 5 days ago

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

simonmacarthur commented 5 days ago

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 commented 2 days ago

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...