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

[Bug] quantum/action_util.c: int8_t oneshot_layer_data breaks get_oneshot_layer #20418

Closed
2 tasks
labre opened this issue Apr 12, 2023 · 1 comment
Closed
2 tasks

Comments

@labre
Copy link
Contributor

labre commented Apr 12, 2023

Describe the Bug

Using an integer instead of an unsigned integer introduces C library implementation defined behaviour. Usually right shifting will fill up with ones then, e.g. on newlib. This behaviour then breaks get_oneshot_layer, which can be found a few lines below that declaration, because the oneshot layer is returned including the filled up ones.

Example:

set_oneshot_layer(1, ONESHOT_START); // ONESHOT_START is 0b11
//                            layer state
// oneshot_layer_data is now: 00001 011
get_oneshot_layer();
// returns 11101

That’s a bug. Fixed by changing the declaration to uint8_t. I’ll prepare a PR. Since the sign is never used, this is simply a typo. uint8_t is the only type, that makes sense here.

Keyboard Used

keyboards/moonlander

Link to product page (if applicable)

https://www.zsa.io/moonlander/

Operating System

Gentoo Linux

qmk doctor Output

Ψ QMK Doctor is checking your environment.
Ψ CLI version: 1.1.1
Ψ QMK home: /mnt/platz/QMK/qmk_firmware
Ψ Detected Linux.
Ψ Git branch: labre_neo2
Ψ Repo version: 0.17.5
⚠ Git has unstashed/uncommitted changes.
⚠ The official repository does not seem to be configured as git remote "upstream".
☒ Can't find avr-gcc in your path.
Would you like to install dependencies? [Y/n] y
This script will make a USE change in order to ensure that that QMK works on your system.
All changes will be sent to the file /etc/portage/package.use/qmkfirmware -- please review it, and read Portage's output carefully before installing any packages on your system.
You will also need to ensure that your kernel is compiled with support for the microcontroller that you are using (e.g. enable Arduino for the Pro Micro). Further information can be found on the Gentoo wiki.
☒ Can't find avr-gcc in your path.
Ψ Found arm-none-eabi-gcc version 10.3.1
Ψ Found avrdude version 7.0
Ψ Found dfu-util version 0.11
Ψ Found dfu-programmer version 1.0.0
Ψ Submodules are up to date.
Ψ Major problems detected, please fix these problems before proceeding.
Ψ Check out the FAQ (https://docs.qmk.fm/#/faq_build) or join the QMK Discord (https://discord.gg/Uq7gcHh) for help.
Note: I’m not using an Arduino board currently, so the error is not critical.

Is AutoHotKey / Karabiner installed

  • AutoHotKey (Windows)
  • Karabiner (macOS)

Other keyboard-related software installed

No response

Additional Context

No response

labre added a commit to labre/qmk_firmware that referenced this issue Apr 12, 2023
Since all bits are used for data encoding, this must not be
signed. int8_t also breaks get_oneshot_layer(), because bit shifting
on signed integers is implementation defined, so right-shift is filled
up with 1s.

closes: qmk#20418
labre added a commit to labre/qmk_firmware that referenced this issue Apr 12, 2023
Since all bits of oneshot_layer_data are used for data encoding, the
variable must not be signed. int8_t also breaks get_oneshot_layer() on
newlib, because bit shifting on signed integers is implementation
defined, so right-shift is filled up with 1s. The only reason this
might work elsewhere are implementations filling up with 0.

closes: qmk#20418
@drashna drashna closed this as completed Apr 12, 2023
@RohitWaghmar
Copy link

⚠ Git has unstashed/uncommitted changes.
⚠ The official repository does not seem to be configured as git remote "upstream".
☒ Can't find arm-none-eabi-gcc in your path.
☒ Can't find avr-gcc in your path.
☒ Can't find avrdude in your path.
☒ Can't find dfu-programmer in your path.
☒ Can't find dfu-util in your path.

please give me solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants