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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Mar 1, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions tmk_core/common/chibios/printf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ static int a2d(char ch) {
return -1;
}

static char a2i(char ch, char** src, int base, int* nump) {
char* p = *src;
static char a2i(char ch, const char** src, int base, int* nump) {
const char* p = *src;
int num = 0;
int digit;
while ((digit = a2d(ch)) >= 0) {
Expand All @@ -119,7 +119,7 @@ static void putchw(void* putp, putcf putf, int n, char z, char* bf) {
while ((ch = *bf++)) putf(putp, ch);
}

void tfp_format(void* putp, putcf putf, char* fmt, va_list va) {
void tfp_format(void* putp, putcf putf, const char* fmt, va_list va) {
// This used to handle max of 12, but binary support jumps this to at least 32
char bf[36];

Expand Down Expand Up @@ -211,19 +211,23 @@ void init_printf(void* putp, void (*putf)(void*, char)) {
stdout_putp = putp;
}

void tfp_printf(char* fmt, ...) {
int tfp_printf(const char* fmt, ...) {
va_list va;
va_start(va, fmt);
tfp_format(stdout_putp, stdout_putf, fmt, va);
va_end(va);

return 1;
}

static void putcp(void* p, char c) { *(*((char**)p))++ = c; }

void tfp_sprintf(char* s, char* fmt, ...) {
int tfp_sprintf(char* s, const char* fmt, ...) {
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: https://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 •
edited
Loading

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.

}
6 changes: 3 additions & 3 deletions tmk_core/common/chibios/printf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ regs Kusti, 23.10.2004

void init_printf(void* putp, void (*putf)(void*, char));

void tfp_printf(char* fmt, ...);
void tfp_sprintf(char* s, char* fmt, ...);
int tfp_printf(const char* fmt, ...);
int tfp_sprintf(char* s, const char* fmt, ...);

void tfp_format(void* putp, void (*putf)(void*, char), char* fmt, va_list va);
void tfp_format(void* putp, void (*putf)(void*, char), const char* fmt, va_list va);

#define printf tfp_printf
#define sprintf tfp_sprintf
Expand Down