AndunHH / spacemouse

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

send_message is called irrespective of user input. #33

Closed simonmacarthur closed 1 week ago

simonmacarthur commented 1 week ago

The main loop calls the send_command every time, but it only needs to be called when one of the velocity or keyOut values are non-zero.

My experience was that by only sending when there was actual user input improved the reliability / performance of the 3Dconnexion training software. (I do not have a real Spacemouse to validate, but presume for power saving reasons it would also wait for input before sending to the USB, and probably not send "do nothing" commands.)

The other slight benefit was being able to put a debug point in that only fired when data was sent.

https://github.com/simonmacarthur/spacemouse/blob/SM-Check-For-User-Input-Before-send_message/spacemouse-keys/spacemouse-keys.ino

AndunHH commented 1 week ago

I like your idea! For the keys: They also need to send a HID report, when the key is released. For the translation and rotation: I suggest sending changed values and not every "not-zero" value.

Additionally, I moved your suggestion to the sendreport() function to keep the main loop a little bit cleaner.

Please review the suggestion I posted above in the pullrequest #34 .

simonmacarthur commented 1 week ago

Looks good to me, and works as expected 😎 ( and the memcmp is certainly neater - I've spent too much time writing c# 🤣 )

Makes sense for the buttons too. I haven't built the hardware with them, so didn't test, but thinking about it that would correlate with the underlying "WM_KEYDOWN / WM_KEYUP" Windows Messaging, so would be needed 👍

My only comment is around debugging. Personally I found it easier to test my hardware by having the serial output only fire on input (you can better compare different axes when the terminal has not been scrolled away with 0 values in between activations). Maybe have send_message return a bool to indicate if a message was sent?

Saying that I think the debugging in general could do with some streamlining.

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

AndunHH commented 1 week ago

That are many good suggestions in one comment ;) I try to seperate them:

Maybe have send_message return a bool to indicate if a message was sent? -> Yes, in fact a good idea!

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.

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

I create a new, open issue with this suggestion.

AndunHH commented 1 week ago

I had to update my pull request #34 because the pc needs regular data from the space mouse or the parts will stop moving... Please check it, there.

simonmacarthur commented 1 week ago

Strange - I didn't experience the mouse stop working, even after hours of inactivity. (I'm on Windows 10, and have been testing using Fusion and the Trainer app).

Happy to confirm with the new changes.