Skip to content

hal: Update matrix keyboard and lcd components to become 64-bit clean #4744

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

Merged
grandixximo merged 1 commit into from
Nov 1, 2026
Merged

hal: Update matrix keyboard and lcd components to become 64-bit clean #4744

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
94 changes: 57 additions & 37 deletions src/hal/components/lcd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ MODULE_DESCRIPTION("Hal-to-text component for Mesa 7I73 and similar");
MODULE_LICENSE("GPL");

#define MAX_CHAN 16
#define MAX_ENTRY 20
#define MAX_ENTRY 22 // Octal of 64-bit is 22 digits long

typedef struct {
hal_refs_u *args;
Expand All @@ -47,7 +47,7 @@ typedef struct {

typedef struct {
lcd_page_t *pages;
unsigned num_pages;
rtapi_uint num_pages;
hal_uint_t page_num;
hal_uint_t out;
hal_real_t contrast;
Expand Down Expand Up @@ -104,16 +104,20 @@ int rtapi_app_main(void){

// allocate shared memory for data
lcd = hal_malloc(sizeof(lcd_t));
if (lcd == 0) {
rtapi_print_msg(RTAPI_MSG_ERR,
"lcd component: Out of Memory\n");
if (NULL == lcd) {
rtapi_print_msg(RTAPI_MSG_ERR, "lcd: Out of Memory\n");
hal_exit(comp_id);
return -1;
}

// Count the instances. Very unlikely to be more than one, but...
for (lcd->num_insts = 0; fmt_strings[lcd->num_insts];lcd->num_insts++){}
lcd->insts = hal_malloc(lcd->num_insts * sizeof(lcd_inst_t));
if (NULL == lcd->insts) {
rtapi_print_msg(RTAPI_MSG_ERR, "lcd: Out of Memory\n");
hal_exit(comp_id);
return -1;
}

for (i = 0; i < lcd->num_insts; i++){
lcd_inst_t *inst = &lcd->insts[i];
Expand All @@ -124,6 +128,11 @@ int rtapi_app_main(void){
if (fmt_strings[i][f] =='|') inst->num_pages++;
}
inst->pages = hal_malloc(inst->num_pages * sizeof(lcd_page_t));
if (NULL == inst->pages) {
rtapi_print_msg(RTAPI_MSG_ERR, "lcd: Out of Memory\n");
hal_exit(comp_id);
return -1;
}

//second pass
f1 = k = p = 0;
Expand All @@ -143,6 +152,11 @@ int rtapi_app_main(void){
inst->pages[p].length = f - f1 + 2;
inst->pages[p].args = hal_malloc(inst->pages[p].num_args
* sizeof(*inst->pages[p].args));
if (NULL == inst->pages[p].fmt || NULL == inst->pages[p].args) {
rtapi_print_msg(RTAPI_MSG_ERR, "lcd: Out of Memory\n");
hal_exit(comp_id);
return -1;
}
f1 = f + 2;
{
int a = -1, s = -1;
Expand All @@ -165,7 +179,7 @@ int rtapi_app_main(void){
break;
case 'u':
case 'c':
retval = hal_pin_new_ui32(comp_id, HAL_IN,
retval = hal_pin_new_uint(comp_id, HAL_IN,
&(inst->pages[p].args[a].u), 0,
"lcd.%02i.page.%02i.arg.%02i",
i, p, a);
Expand All @@ -175,7 +189,7 @@ int rtapi_app_main(void){

break;
case 's':
retval = hal_pin_new_si32(comp_id, HAL_IN,
retval = hal_pin_new_sint(comp_id, HAL_IN,
&(inst->pages[p].args[a].s), 0,
"lcd.%02i.page.%02i.arg.%02i",
i, p, a);
Expand Down Expand Up @@ -207,13 +221,13 @@ int rtapi_app_main(void){
}

for (i = 0; i < lcd->num_insts; i++){
retval = hal_pin_new_ui32(comp_id, HAL_IN, &(lcd->insts[i].page_num), 0,
retval = hal_pin_new_uint(comp_id, HAL_IN, &(lcd->insts[i].page_num), 0,
"lcd.%02i.page_num", i);
if (retval != 0) {
return retval;
}
lcd->insts[i].last_page = -1; // force screen refresh
retval = hal_pin_new_ui32(comp_id, HAL_OUT, &(lcd->insts[i].out), 0,
retval = hal_pin_new_uint(comp_id, HAL_OUT, &(lcd->insts[i].out), 0,
"lcd.%02i.out",i);
if (retval != 0) {
return retval;
Expand All @@ -223,7 +237,7 @@ int rtapi_app_main(void){
if (retval != 0) {
return retval;
}
retval = hal_param_new_ui32(comp_id, HAL_RW, &(lcd->insts[i].dp), '.',
retval = hal_param_new_uint(comp_id, HAL_RW, &(lcd->insts[i].dp), '.',
"lcd.%02i.decimal-separator",i);
if (retval != 0) {
return retval;
Expand Down Expand Up @@ -276,18 +290,18 @@ static void write_one(lcd_inst_t *inst){
return;}

if (inst->buff[inst->c_ptr] != 0){
hal_set_ui32(inst->out, inst->buff[inst->c_ptr++]);
hal_set_uint(inst->out, inst->buff[inst->c_ptr++]);
return;
}

inst->c_ptr = 0;
inst->buff[0] = 0;

if (hal_get_ui32(inst->page_num) >= inst->num_pages) return; // should this error?
if (hal_get_uint(inst->page_num) >= inst->num_pages) return; // should this error?

if (hal_get_ui32(inst->page_num) != inst->last_page){
inst->last_page = hal_get_ui32(inst->page_num);
hal_set_ui32(inst->out, 0x11); //cursor off
if (hal_get_uint(inst->page_num) != inst->last_page){
inst->last_page = hal_get_uint(inst->page_num);
hal_set_uint(inst->out, 0x11); //cursor off
inst->buff[0] = 0x1E; //cursor home
inst->buff[1] = 0x1A; //clear screen
inst->buff[2] = 0; // end
Expand All @@ -297,8 +311,8 @@ static void write_one(lcd_inst_t *inst){
return;
}

if (inst->f_ptr >= inst->pages[hal_get_ui32(inst->page_num)].length){
hal_set_ui32(inst->out, 0x1B); // ESC
if (inst->f_ptr >= inst->pages[hal_get_uint(inst->page_num)].length){
hal_set_uint(inst->out, 0x1B); // ESC
inst->buff[0] = 0x3D; // =
inst->buff[1] = 0x20; // Line 0
inst->buff[2] = 0x20; // Column 0
Expand All @@ -314,17 +328,17 @@ static void write_one(lcd_inst_t *inst){
if (c > 0xBF) c = 0xBF;
if (c < 0x20) c = 0x20;
inst->last_contrast = hal_get_real(inst->contrast);
hal_set_ui32(inst->out, 0x1B);
hal_set_uint(inst->out, 0x1B);
inst->buff[0] = 'C';
inst->buff[1] = c;
inst->buff[2] = 0;
inst->c_ptr = 0;
return;
}

switch (inst->pages[hal_get_ui32(inst->page_num)].fmt[inst->f_ptr]){
switch (inst->pages[hal_get_uint(inst->page_num)].fmt[inst->f_ptr]){
case '\\': //escape chars
c1 = inst->pages[hal_get_ui32(inst->page_num)].fmt[++inst->f_ptr];
c1 = inst->pages[hal_get_uint(inst->page_num)].fmt[++inst->f_ptr];
switch (c1){
case 'n':
case 'N':
Expand All @@ -350,7 +364,7 @@ static void write_one(lcd_inst_t *inst){
/* Fallthrough */

default: //check for hex
c2 = inst->pages[hal_get_ui32(inst->page_num)].fmt[++inst->f_ptr];
c2 = inst->pages[hal_get_uint(inst->page_num)].fmt[++inst->f_ptr];
inst->f_ptr++;
if (c1 > '9') c1 &= 0xDF; //upper case
if (c2 > '9') c2 &= 0xDF;
Expand All @@ -362,32 +376,32 @@ static void write_one(lcd_inst_t *inst){
inst->buff[0] = 0;
}
}
hal_set_ui32(inst->out, inst->buff[0]);
hal_set_uint(inst->out, inst->buff[0]);
inst->c_ptr = 1;
return;
case '%':
retval = parse_fmt(inst->pages[hal_get_ui32(inst->page_num)].fmt,
retval = parse_fmt(inst->pages[hal_get_uint(inst->page_num)].fmt,
&inst->f_ptr,
inst->buff,
&(inst->pages[hal_get_ui32(inst->page_num)].args[inst->a_ptr++]),
(char)hal_get_ui32(inst->dp));
&(inst->pages[hal_get_uint(inst->page_num)].args[inst->a_ptr++]),
(char)hal_get_uint(inst->dp));
if (retval >= 0) {
hal_set_ui32(inst->out, inst->buff[0]);
hal_set_uint(inst->out, inst->buff[0]);
inst->c_ptr = 1;
inst->f_ptr++;
return;
}
/* Fallthrough */
default:
hal_set_ui32(inst->out, inst->pages[hal_get_ui32(inst->page_num)].fmt[inst->f_ptr++]);
hal_set_uint(inst->out, inst->pages[hal_get_uint(inst->page_num)].fmt[inst->f_ptr++]);
}
}

int num_digits_baseN(rtapi_u64 val, int base){
static int num_digits_baseN(rtapi_u64 val, int base){
int n = 1;
rtapi_u64 m = 1;
while ((m *= base) <= val){
n += 1;
while(val >= (rtapi_u64)base) {
val /= base;
n++;
}
return n;
}
Expand Down Expand Up @@ -473,9 +487,9 @@ static int parse_fmt(char *in, int *ptr, char *out, hal_refs_u *val, char dp){
case 'u':
if (out == NULL || val == NULL) return 'u';
{
unsigned int tmp;
rtapi_uint tmp;
int i;
unsigned int v = hal_get_ui32(val->u);
rtapi_uint v = hal_get_uint(val->u);

if (c < 1) c = num_digits_baseN(v, base);

@grandixximo grandixximo Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that the full 64-bit value reaches num_digits_baseN(), can m *= base wrap? With %x and a value >= 16^15, I think m becomes 0 and the loop never ends in the RT thread (%o likewise for >= 2^63).

And with %o, a 64-bit value can need 22 digits; is anything clamping c to MAX_ENTRY here, or does the digit loop run past buff?

Counting by division would avoid the wrap:

while (val >= (rtapi_u64)base) {
    val /= base;
    n++;
}

@BsAtHome BsAtHome Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good one. Changed.

There are still some "gaps" in the conversion. The floating point conversion uses a "blind" conversion to integer and it hopes to fit into a 64-bit value when it counts the digits. Don't know if it is worth the hassle to fix all the problems in lcd.

@grandixximo grandixximo Oct 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, the hang is gone. The second half of my question still stands, I think: with %o and a value >= 2^60, num_digits_baseN() returns 21 or 22, and nothing clamps c before out[c] = 0, so it writes past buff[MAX_ENTRY + 1]. Would one line after the width calculation be enough? The digit loop already prints * when the value does not fit:

                if (c < 1) c = num_digits_baseN(v, base);
                if (c > MAX_ENTRY) c = MAX_ENTRY;

On the float conversion, I wondered the same, so I pulled parse_fmt() into a small userspace harness with ASan and UBSan. The float path is unchanged from master, so none of this comes from your PR, but I think one part is worth fixing anyway, because a plain pin value reaches it:

  • %f with |value| >= 1e15 (default 4 decimals) gives c > MAX_ENTRY, and the goto overflow path then fills * up to the unclamped c, so it writes past buff. Same for NaN, where the conversion yields 2^63 on x86.
  • 1e300 and inf print 0.0000: the "blind" conversion you mention is UB and comes back 0 here.
  • From the format string only: %.25f reads past pow10[], and %5.9f (precision not smaller than width) writes before buff.

Would something like this be enough? It keeps the existing overflow path and only feeds it a clamped width:

                if (m < 0) m = 4;
                if (m > MAX_ENTRY - 2) m = MAX_ENTRY - 2;

                if (c < 1) c = v < 1e19 ? num_digits_baseN(v, base) + m + 1 + s : MAX_ENTRY;

                if (c > MAX_ENTRY || c < m + 2 || !(v * pow10[m] < 1e19)){ // then it won't fit, also inf and NaN
                    if (c > MAX_ENTRY) c = MAX_ENTRY;
                    tmp = 2;
                    goto overflow;
                }

With both changes the harness is clean on 25 cases (64-bit %u %x %o %d, %f from 3.14 to 1e300, inf, NaN, %.25f, %5.9f, %06.2f), and the normal outputs are unchanged. Values that do not fit now show **** instead of corrupting memory or showing 0.0000. I am fine with this here or as a separate PR, whichever you prefer.

@BsAtHome BsAtHome Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The MAX_ENTRY needs to encompass the %o format and thus needs to be ceil(64/3) = 22 or more.

The FP question,... Well, the thing is that the calculation will overflow badly when the value is >= 2^64 because the digit calculation truncates the actual value in a cast from double to rtapi_u64. The most obvious overflow is from %.21f and beyond because it indexes the pow10[] array out of bounds. So a check on m >= NELEM(pow10)` and go to overflow can trivially prevent that.

You are right that there are more things that need to be done. I'm not sure it is worth the effort to do much more in this PR as the real problem has been present for a long time. FWIW, the code should probably use rtapi_vsnprintf derived code instead.

@grandixximo grandixximo Oct 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, %o is fine now with MAX_ENTRY 22, the harness agrees.

On the new m check: I think the goto overflow comes before c gets a value, or am I missing something? With %.25f and no width, c is still -1, so out[c] = 0 writes one byte before buff; ASan flags it. Giving it the full width first fixes it in the harness:

                if (m >= (int)(sizeof(pow10) / sizeof(*pow10))) {
                    if (c < 1) c = MAX_ENTRY;
                    tmp = 2;
                    goto overflow;
                }

Agreed on leaving the rest out of this PR, since it is old. For the record, what still shows up in the harness: %f with values >= 1e18 and NaN write past buff through the unclamped c in the overflow path, %5.9f writes before it, and 1e300, inf and %.20f of 1 print zeros.

@BsAtHome BsAtHome Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good you are looking. Missed that one.

But is is also an argument to spit the printable string format into fractions at load time that are passed to rtapi_snprintf() part-by-part with the correct argument and the full result constructed that way. It would prevent using the reinvention of the wheel as a square with two sides missing ;-)


Expand All @@ -501,16 +515,17 @@ static int parse_fmt(char *in, int *ptr, char *out, hal_refs_u *val, char dp){
case 'i':
if (out == NULL || val == NULL) return 's';
{
int tmp, s = 0;
rtapi_sint tmp;
int s = 0;
int i;
int v = hal_get_si32(val->s);
rtapi_sint v = hal_get_sint(val->s);

if (sgn == '+') s = 1;
if (v < 0) {s = 1; sgn = '-'; v = -v;}

if (c < 1) c = num_digits_baseN(v, base) + s;

tmp = abs(v);
tmp = v < 0 ? -v : v;
for (i = c - 1; i >= s; i--){
if (tmp != 0 || i == c - 1){
out[i] = digits[tmp % 10];
Expand Down Expand Up @@ -550,6 +565,11 @@ static int parse_fmt(char *in, int *ptr, char *out, hal_refs_u *val, char dp){
if (v < 0) {s = 1; sgn = '-'; v = -v;}

if (m < 0) m = 4;
if (m >= (int)(sizeof(pow10) / sizeof(*pow10))) {
if (c < 1) c = MAX_ENTRY;
tmp = 2;
goto overflow;
}

if (c < 1) c = num_digits_baseN(v, base) + m + 1 + s;

Expand Down Expand Up @@ -607,7 +627,7 @@ static int parse_fmt(char *in, int *ptr, char *out, hal_refs_u *val, char dp){
if (out == NULL || val == NULL) return 'c';
{
int i;
unsigned char v = hal_get_ui32(val->u);
unsigned char v = hal_get_uint(val->u) & 0xff;

if (c == 0) c = 1;
for (i = 0; i < c; i++){
Expand Down
Loading
Loading