Overflow issue with Roller/Drop Down List set options

Bug (archived)
1

Issue with Roller/Drop Down List set options.

CPU: Xilinx Zynq

What do you experience?

I am creating a large list of ‘wordy’ objects using a roller based on a file downloaded from the Internet. This code has been working happily for many months now, but today the system hung. Upon investigation it seems I have an issue with the function:

void lv_ddlist_set_options(lv_obj_t * ddlist, const char * options);

The variable “i” which is declared on line 188 of lv_ddlist.c is a uint16_t, this limits the maximum text buffer size passed to the function for processing to 64 Kilobytes. In my case I passed about 66 Kilobytes which caused the variable “i” to wrap so the function never returns as it can not advance far enough into the text buffer to find the terminating NULL character.

What do you expect?

Is there any reason why the variable “i” can not be declared as a uint32_t removing this limitation?

Kind Regards,

Pete

2

It seems that you always find the bugs relating to large amounts of text. :slightly_smiling_face:

The root problem here is that there are many internal APIs which use uint16_t to reference a character’s index in a string. Changing uint16_t to uint32_t in lv_ddlist alone will not end well, from what I see.

The cleanest solution I can think of that avoids an excessive memory consumption increase for people is to add a new typedef to lv_conf.h that allows you to choose whether text is indexed using 16-bit or 32-bit variables.

@kisvegabor What do you think about this solution?

1 Like
3

@embeddedt yes sorry about that, it’s just that one of the projects I am currently working on does indeed push the boundaries in those areas… :blush:

To keep my project going for now I have just changed my local copy of littlevgl to use a uint32_t at line 188 of the source file, which appears fine for the time being, while I continue with other things…

If @kisvegabor agrees with your suggestion, that will be absolutely fine with me. :grinning:

Thank you.

Pete

1 Like
4

My suggestion can only safely be applied to dev-7.0 (as it changes the ABI and API) so you’ll have to stick with your patch or backport mine.

1 Like
5

Hi @embeddedt,

Thank you for the update, if you can let me know when you get around to applying the fix to dev-7.0 I would much appreciate it. I will then look at updating my projects to version 7.0 at that point.

Kind Regards,

Pete

6

Do you think the increase in memory usage would be notable? Right now I don’t remember too many places where string indexes are stored directly. They are rather only local variables and function parameters. However, the number of options, rows or similar things are usually uint16_t and they are stored in objects’ ext data. Do you think this limit can be reached as well?

7

I can’t imagine any user interface where someone would want to scroll through 65,536 items to find what they were looking for.

The memory usage probably won’t be too much of a problem; I was thinking more about performance. On 16-bit platforms, I don’t know how much more expensive passing a 32-bit variable between functions is when compared to 16-bit variables.

8

Fine, I agree.

As far as I know, it simply takes 2 registers to pass 32-bit variables. However, I’m sure it won’t cause any notable difference.

10

I’ll apply this fix within the next few days.

1 Like
11

Is there any downside if using size_t instead of uint32_t? AFAIK They are the same size (in bytes) on 32bit CPUs, like ARM Cortex M, most functions related to sizes, or lenghts, uses size_t.

12

uint32_t seems to be used in a number of spots elsewhere in the text code, so I’d rather stick with that for consistency. On a 64-bit CPU, size_t can be 8 bytes, and that wouldn’t match the uint32_t values already in use.

13

Hi @embeddedt,

Hope you are well!

I was wondering if you have had a chance to look at this as yet?

Sorry to push but I have now updated one of my projects to V7.0 and am now seeing a number of issues caused by this limitation which are breaking my user interface. If you are overwhelmed currently and haven’t made a start on the changes, would you like me to have a go at doing them? As far as I can tell you intend to make a new type define for text indexing throughout the code which can be configured for uint16_t or uint32_t depending on user requirements. I could do this if necessary, as far as I can see so far it would involve the files, lv_draw_label.c, lv_dropdown.c lv_roller.c, lv_label.c and lv_txt.c. There may be other files also but haven’t looked that deeply yet. If you want me to take this on, should the new type be something along the lines of lv_txt_idx_t?

Look forward to your comments.

Kind Regards,

Pete

14

Hi @pete-pjb; thanks for your willingness to take this on. I appreciate it. :slightly_smiling_face:

I had started making the change on the dropdown file a few weeks ago but got quite busy and hadn’t been able to finish it. Sorry for leaving you waiting. I pushed my existing changes to a new 32bit_idx branch on the repository (based on 7.0 master), so you could start there.

@kisvegabor suggested that we just use 32-bit indexes across the board, as it won’t affect performance too badly. That means it should be a matter of replacing uint16_t with uint32_t in these files.

15

Hi @embeddedt,

Thanks for your prompt reply, no worries :slight_smile:, I have cloned the 32bit_idx branch and merged the latest master with it, I will continue your work and create a pull request when I am done.

Kind Regards,

Pete

1 Like