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

Small fixes for LCD text printing #155

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Small fixes for LCD text printing #155

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2eed223
Fix buffer overflow when trying to display long GitVersion
nica-f Jan 11, 2019
cd13fa3
Make one digit more space for offset setup values (caused buffer over…
nica-f Jan 11, 2019
38f181c
Revert "Fix buffer overflow when trying to display long GitVersion"
Jan 20, 2019
21446e5
Limit Version_GetGitVersion() print to what can be actually displayed
Jan 20, 2019
fcdffd1
Merge branch 'UnifiedEngineering:master' into master
nica-f Mar 31, 2022
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
4 changes: 2 additions & 2 deletions src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ int main(void) {
LCD_disp_str((uint8_t*)buf, len, 0, 64 - 6, FONT6X6);
printf("\nRunning on an %s", buf);

len = snprintf(buf, sizeof(buf), "%s", Version_GetGitVersion());
Copy link

Choose a reason for hiding this comment

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

This introduces a potential buyer overflow for future devs, try sizeof(buf) - 1

Copy link
Author

Choose a reason for hiding this comment

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

Oh wow, that was fast

As far as I have understood the original problem, buf is not the
problem. The problem is that the LCD_disp_str() function also uses fixed
length buffers according to display pixel size and these buffers will
overflow if you try to print texts resulting in more pixels than the
display width. At a 6x6 font the maximum is 21 characters, that's why I
limited to it.

Does that explain it a bit?

Copy link

Choose a reason for hiding this comment

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

In that case, I would suggest introducing a common define used between the output code and buffer allocation, which makes the relationship explicit

len = snprintf(buf, 21, "%s", Version_GetGitVersion());
LCD_disp_str((uint8_t*)buf, len, 128 - (len * 6), 0, FONT6X6);

LCD_FB_Update();
Expand Down Expand Up @@ -348,7 +348,7 @@ static int32_t Main_Work(void) {
len = snprintf(buf, sizeof(buf), "T-962 controller");
LCD_disp_str((uint8_t*)buf, len, LCD_ALIGN_CENTER(len), 0, FONT6X6);

len = snprintf(buf, sizeof(buf), "%s", Version_GetGitVersion());
len = snprintf(buf, 21, "%s", Version_GetGitVersion());
LCD_disp_str((uint8_t*)buf, len, LCD_ALIGN_CENTER(len), 64 - 6, FONT6X6);

LCD_BMPDisplay(stopbmp, 127 - 17, 0);
Expand Down
4 changes: 2 additions & 2 deletions src/setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ static setupMenuStruct setupmenu[] = {
{"Min fan speed %4.0f", REFLOW_MIN_FAN_SPEED, 0, 254, 0, 1.0f},
{"Cycle done beep %4.1fs", REFLOW_BEEP_DONE_LEN, 0, 254, 0, 0.1f},
{"Left TC gain %1.2f", TC_LEFT_GAIN, 10, 190, 0, 0.01f},
{"Left TC offset %+1.2f", TC_LEFT_OFFSET, 0, 200, -100, 0.25f},
{"Left TC offs. %+1.2f", TC_LEFT_OFFSET, 0, 200, -100, 0.25f},
{"Right TC gain %1.2f", TC_RIGHT_GAIN, 10, 190, 0, 0.01f},
{"Right TC offset %+1.2f", TC_RIGHT_OFFSET, 0, 200, -100, 0.25f},
{"Right TC offs. %+1.2f", TC_RIGHT_OFFSET, 0, 200, -100, 0.25f},
};
#define NUM_SETUP_ITEMS (sizeof(setupmenu) / sizeof(setupmenu[0]))

Expand Down