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

Short term fix for conflicting types for 'tfp_printf' #8157

Merged
merged 1 commit into from
Mar 1, 2020

Conversation

zvecr
Copy link
Member

@zvecr zvecr commented Feb 12, 2020

Description

When adding encoders to a board, while console is enabled, the following error is thrown

Compiling: quantum/encoder.c                                                                       In file included from tmk_core/common/print.h:76:0,
                 from tmk_core/common/debug.h:22,
                 from quantum/keymap.h:35,
                 from quantum/quantum.h:30,
                 from quantum/encoder.h:20,
                 from quantum/encoder.c:18:
tmk_core/common/chibios/printf.h:107:16: error: conflicting types for 'tfp_printf'
 #define printf tfp_printf
                ^
tmk_core/common/chibios/printf.h:102:6: note: previous declaration of 'tfp_printf' was here
 void tfp_printf(char* fmt, ...);
      ^~~~~~~~~~
tmk_core/common/chibios/printf.h:108:17: error: conflicting types for 'tfp_sprintf'
 #define sprintf tfp_sprintf
                 ^
tmk_core/common/chibios/printf.h:103:6: note: previous declaration of 'tfp_sprintf' was here
 void tfp_sprintf(char* s, char* fmt, ...);
      ^~~~~~~~~~~
 [ERRORS]

The route cause, tfp_printf/tfp_sprintf gets a macro alias to increase compatibility, however the signature of the tfp versions are not compliant.

For reference, ive used http://www.cplusplus.com/reference/cstdio/printf/ and http://www.cplusplus.com/reference/cstdio/snprintf/, to update tmk_core/common/chibios/printf.h.

Types of Changes

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

Checklist

  • My code follows the code style of this project.
  • 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).

va_list va;
va_start(va, fmt);
tfp_format(&s, putcp, fmt, va);
putcp(&s, 0);
va_end(va);

return 1;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was a void before, so no one can be checking the return code....

Question is, while not perfect, will this do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this: http://www.cplusplus.com/reference/cstdio/printf/ the standard seems to be to return the number of characters written, or a negative number if unsuccessful. If tfp_format() keeps track of that already, we could just change its return type too, and use that. Though I don't know how useful this information is, so returning 1 for now might be good enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a more invasive change, things like uli2a would need to be updated to return a count of written characters, then tfp_format would need keep the tally. tfp_sprintf could potentially return strlen of the output buffer if it wants to be slightly more accurate. To be 100% compliant, we would need to do all the extra error handling.

At the moment, I kinda just want my board to compile... though I do see long term value in it being feature complete.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can use this fork of tinyprintf instead: https://github.com/cjlano/tinyprintf
It seems to have fixed a few bugs and added more functions, plus tfp_sprintf() returns the number of characters already!

Copy link
Member Author

@zvecr zvecr Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still has the tfp_printf void issue, and we would need to port the binary support into it. Worth considering, plus theres alternatives like https://github.com/mpaland/printf which tick both non void functions and binary support.

Edit: prepared a branch with the mpaland implementation https://github.com/qmk/qmk_firmware/compare/master...zvecr:feature/mpaland_printf?expand=1, seems to work fine with brief testing but it does increase chibios based firmware by 750 bytes.

@zvecr zvecr requested a review from a team February 12, 2020 23:52
@zvecr zvecr marked this pull request as ready for review February 26, 2020 22:07
@zvecr zvecr force-pushed the feature/chibios_printf_conflicting branch from 5beea64 to ae701e8 Compare February 26, 2020 22:25
@zvecr zvecr changed the title [WIP] fix for conflicting types for 'tfp_printf' Short term fix for conflicting types for 'tfp_printf' Feb 26, 2020
@zvecr zvecr mentioned this pull request Feb 29, 2020
13 tasks
@zvecr zvecr force-pushed the feature/chibios_printf_conflicting branch from ae701e8 to 2948fdf Compare March 1, 2020 02:51
@zvecr zvecr removed the in progress label Mar 1, 2020
@fauxpark fauxpark merged commit e7fb873 into qmk:master Mar 1, 2020
rhruiz pushed a commit to rhruiz/qmk_firmware that referenced this pull request Mar 1, 2020
Shinichi-Ohki added a commit to Shinichi-Ohki/qmk_firmware that referenced this pull request Mar 3, 2020
* 'master' of https://github.com/qmk/qmk_firmware: (37 commits)
  Update Hungarian keymap and add sendstring LUT (qmk#8220)
  Remove "ugly hack in usb_main.c" comments (qmk#8296)
  Update encoder functions for Iris VIA keymap (qmk#8295)
  Reduce PROGMEM usage for sendstring LUT (qmk#8109)
  [Docs] Update ISP Flashing guide (qmk#8149)
  Rewrite the Bathroom Epiphanies Frosty Flake matrix and LED handling (qmk#8243)
  Add onekey keymap for testing reset to bootloader. (qmk#8288)
  Get the direction right on the S75 encoder (qmk#8287)
  Prune out pure software pwm && custom driver && remove wrapping BACKLIGHT_PIN (qmk#8041)
  Make a fix to savage65 and tmov2 for via (qmk#8286)
  format code according to conventions [skip ci]
  Short term fix for conflicting types for 'tfp_printf' (qmk#8157)
  Fix recent clang-format breaking quantum.c (qmk#8282)
  format code according to conventions [skip ci]
  Remove duplicate BRTG case (qmk#8277)
  Clean up includes for glcdfont headers (qmk#7745)
  Fix the Breaking Changes doc again
  [Docs] translated 'feature_tap_dance.md' to japanese. (qmk#8137)
  PWM DMA based RGB Underglow for STM32 (qmk#7928)
  Add VIA support to Prime_M. Clean up all files (qmk#8247)
  ...
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Mar 5, 2020
c0psrul3 pushed a commit to c0psrul3/qmk_firmware that referenced this pull request Mar 23, 2020
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request Apr 2, 2020
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
@zvecr zvecr deleted the feature/chibios_printf_conflicting branch April 28, 2020 01:15
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request May 24, 2020
format code according to conventions [skip ci]
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request May 24, 2020
format code according to conventions [skip ci]
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Jun 12, 2020
format code according to conventions [skip ci]
jakeisnt pushed a commit to jakeisnt/qmk_firmware that referenced this pull request Aug 20, 2020
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants