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

[Core][Bugfix] Joystick: Ignore axis-related code when axes count=0 #16983

Closed
wants to merge 4 commits into from

Conversation

amrsoll
Copy link

@amrsoll amrsoll commented May 1, 2022

The analog joystick feature does not compile accross all platforms (see #4226 (review))

All joystick related code is put between #if JOYSTICK_AXES_COUNT > 0

This also reduces the size of the compiled code when you only want to use joystick buttons.

How to reproduce bug:

  1. qmk new-keyboard
  2. select the MCU GD32VF103 (other options don't matter)
  3. rules.mk Add JOYSTICK_ENABLE = yes
  4. config.h : Add
    #define JOYSTICK_BUTTON_COUNT 1
    #define JOYSTICK_AXES_COUNT 0
  5. (optional) Change a key to JS_BUTTON0 in keymap
  6. run qmk compile against new keyboard

Errors will show related to the ADC.

Disclaimer

  1. I do not have the proper HW to debug on all keyboards, but it does work from the RP2040 branch which has a similar problem
    TBH, I'm mostly submitting this PR because it allows me to 🙈 on the bug I described in [Core] Add Raspberry Pi RP2040 support #14877 (comment)
  2. I guess ifeq ($(shell expr $(JOYSTICK_AXES_COUNT) \> 0), 1) ain't so pretty,
    but it didn't work with #ifneq ($(JOYSTICK_AXES_COUNT), 0) ??
  3. I have not been able to test if it actually broke the joystick (axis) feature, but I really can't see how it would.

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

None

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

The analog joystick feature does not compile accross all platforms (see qmk#4226 (review))

All joystick related code is put between #if JOYSTICK_AXES_COUNT > 0

This also reduces the size of the compiled code when you only want to use joystick buttons.

\### How to reproduce bug:

1. `qmk new-keyboard`
2. select the MCU `GD32VF103` (other options don't matter)
3. `rules.mk` Add `JOYSTICK_ENABLE = yes`
4. `config.h` : Add
    ```c
    #define JOYSTICK_BUTTON_COUNT 1
    #define JOYSTICK_AXES_COUNT 0
    ```
5. (optional) Change a key to `JS_BUTTON0` in keymap
6. run `qmk compile` against new keyboard

Errors will show related to the ADC.

\### Disclaimer

1. I do not have the proper HW to debug on all keyboards, but it does work from the RP2040 branch which has a similar problem
   See qmk#14877 (comment)
2. I guess `ifeq ($(shell expr $(JOYSTICK_AXES_COUNT) \> 0), 1)` ain't so pretty,
   but it didn't work with `#ifneq ($(JOYSTICK_AXES_COUNT), 0) ??
@github-actions github-actions bot added the core label May 1, 2022
@amrsoll amrsoll changed the title Ignore axis-related code when axis number=0 [Core][Bugfix] Jystick: Ignore axis-related code when axis number=0 May 1, 2022
@amrsoll amrsoll changed the title [Core][Bugfix] Jystick: Ignore axis-related code when axis number=0 [Core][Bugfix] Jystick: Ignore axis-related code when axes count=0 May 1, 2022
@amrsoll amrsoll changed the title [Core][Bugfix] Jystick: Ignore axis-related code when axes count=0 [Core][Bugfix] Joystick: Ignore axis-related code when axes count=0 May 1, 2022
@drashna drashna requested a review from a team May 1, 2022 18:13
@zvecr
Copy link
Member

zvecr commented May 1, 2022

The savePinState/restorePinState logic is due to be removed, which would supersede this fix.

(Also im fairly certain this wont work, as JOYSTICK_AXES_COUNT will not be available due to it being specified in config.h)

@amrsoll
Copy link
Author

amrsoll commented May 1, 2022

I am looking forward to that! The logic in the joystick parts are a bit confusing to me. Where can I find references to that reformatting, either in plans or in PRs?

@amrsoll
Copy link
Author

amrsoll commented May 1, 2022

Do you mean it won't work in common_features.mk ?

@fauxpark
Copy link
Member

fauxpark commented May 1, 2022

JOYSTICK_AXES_COUNT is a C preprocessor define, not a makefile variable, so it will not be visible at that level.

@amrsoll
Copy link
Author

amrsoll commented May 1, 2022

Then I guess it ain't possible to make that work unless we start defining the number of axis in rules.mk .. Is that advisable?

@fauxpark
Copy link
Member

fauxpark commented May 1, 2022

The change doesn't really make much sense anyway as you can configure axes to not read from the ADC with JOYSTICK_AXIS_VIRTUAL - having a count of 0 is not a good metric for whether or not to compile the ADC code.

@zvecr
Copy link
Member

zvecr commented May 1, 2022

Is that advisable

Nope, should remain config.h only at the moment.

As I mention, 9a86f5c the code causing this issue is going shortly so I dont think we need to go forward with the proposed changes right now.

@amrsoll
Copy link
Author

amrsoll commented May 1, 2022

having a count of 0 is not a good metric for whether or not to compile the ADC code.

It is only avoiding compilation of ADC code for the joystick, as the joystick only uses it for reading analog axes. Doesn't prohibit other features from using the ADC.

As I mention, 9a86f5c the code causing this issue is going shortly so I dont think we need to go forward with the proposed changes right now.

Which branch is it part of? GitHub isn't tracking it anymore (rebased) and I can't find it through search


Perhaps the joystick buttons should be removed in favour of using programmable buttons? I am trying to use both, but they step on each others toes when trying to use with a flight simulator 🙄

@amrsoll
Copy link
Author

amrsoll commented May 1, 2022

Closing as recommended.

@amrsoll amrsoll closed this May 1, 2022
@amrsoll
Copy link
Author

amrsoll commented May 1, 2022

you can configure axes to not read from the ADC

@fauxpark True, but it still tries to compile it, which can break, unless you explicit that the driver is digital, which is a workaround and shouldn't be considered solid because it requires to understand the mechanisms behind.

This PR was aimed at people trying to make a keymap using joystick buttons but no axis, which is indeed particular.

@amrsoll amrsoll reopened this May 1, 2022
@amrsoll amrsoll closed this May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants