Menu
▾
▴
lcd4linux-devel
|
From: <cod...@gm...> - 2012-03-22 14:20:23
Attachments:
lcd4linux.conf.short
|
Hello Michael, in the attachment you'll find a short conf. I hope the curse driver (more simple to config) is ok for you. You'll notice that the displayed number (cnt) will increase in steps of 2 each second (not 1 as expected) because the timer expression is evaluated twice in one timer run. Causes of the problem: The loop in line 367 of 'timer_group.c'. At first the timer widget is added as one-shot timer to the 'TimerGroupWidgets' field (timer_group.c) via 'timer_add_widget' (timer_group.c) called by 'widget_timer_update' called by 'widget_timer_init' (both in 'widget_timer.c'). Ignoring other timers the TimerGroupWidgets will contain: 0: Timer_Cnt active If 'timer_process_group' (timer_group.c) is called it will call 'widget_timer_update' (widget_timer.c) for 0 via callback _in the loop_. 'widget_timer_update' calls 'timer_add_widget' (timer_group.c) which adds a new Timer_Cnt to a new place in TimerGroupWidgets cause the old Timer_Cnt is still active. It'll be set inactive (cause one-shot) in the loop _after_ 'widget_timer_update' already finished. After that we got... 0: Timer_Cnt inactive 1: Timer_Cnt active ... and the loop continues to process the next active Timer_cnt (1), which leads to... 0: Timer_Cnt active 1: Timer_Cnt inactive => For now on Timer_Cnt is contained twice TimerGroupWidgets and will processed twice. First 0 that activates 1 for the same run, which activateds 0 for the next run and so on. Solution: My patch exchanges the timer elements (for the same widget) so the active element will be on the already processed position in TimerGroupWidgets. So it won't be processed again in the same run. (I hope to not create unwished side effects.) Regards Marcus -- NEU: FreePhone 3-fach-Flat mit kostenlosem Smartphone! Jetzt informieren: http://mobile.1und1.de/?ac=OM.PW.PW003K20328T7073a |
|
From: Marcus M. <cod...@gm...> - 2012-03-22 21:30:52
Attachments:
twice-evaluation-2.patch
|
Hello again, the patch offered by me to solve the twice eval. problem is not that effecient. - Sorry for that. I just tried to solve it. The attached one (no new code only the original in new order) should make the new timer be stored at the right place at once instead of exchanging it later like the old patch does. It sets the one_shot widgets inactive after the active-test but before the callback. So in the callback that place is free to store a new element. Regards Marcus -- NEU: FreePhone 3-fach-Flat mit kostenlosem Smartphone! Jetzt informieren: http://mobile.1und1.de/?ac=OM.PW.PW003K20328T7073a |
|
From: Martin Z. <co...@mz...> - 2012-03-23 20:59:45
|
Hi Marcus, thanks for your bug report and two patches! I am the one who is responsible for rewriting the whole timer code about two years ago, and I have also added the timer group code. However, I think that the problem lies in the general way that timers are handled in "lcd4linux" and that it's current manifestation in my timer group code is just one of the possible problems. I do not want to defend my code, it's just that I've always had a bad feeling about the way that the timers are processed in "lcd4linux". See, timers are (in)activated by a setting a variable to a defined state (I have actually *named* these states for easier reading in revision 1182). Inactive timers are reused by simply looping through all timers and looking for the state "inactive". And this is where the problem lies -- while lcd4linux gets a lot faster by not allocating and freeing memory for each and every timer, new timers may end up in any slot. As timers are processed by simply looping through all available timer slots, this may lead to some timers being processed more than they should (if during looping, a new active timer is placed in a slot with a higher number than the loop counter). So while your patch may work fine (I haven't looked at it yet, as I have tried to concentrate on the problem itself), it might be time to fix timer processing instead. We all of a sudden have a problem that we can understand. Just imagine that a similar but much more complex problem creeps up again in a couple of months... :( Here is my solution to the problem (feel free to post your own solutions!): right now timers can have two states, either active or inactive. New timers are set to "active" and this is where the problem lies. I have thought of adding a third state "on hold". New timers should be set to "on hold" to make sure they are not processed in the current loop. Now, each time "widget_timer_update()" is called, it should change all timers from "on hold" to "active" so will get processed from then on. Any thoughts on this from you or anyone (Michael)? Best regards, Martin -- www.mzuther.de www.radix-musik.de |
|
From: Marcus M. <cod...@gm...> - 2012-03-25 09:37:52
|
Hello Martin, thanks for your feedback and ideas. Till now I saw the bug located in the timer_group.c only. But due to the same problem in timer.c (later field elements might be changed in loop) I fear my patches might fail if the interval of a widget is changed. Both files will have to be patched similar to avoid the bug. > I have thought of adding a third state "on hold". New timers > should be set to "on hold" to make sure they are not processed in the > current loop. Now, each time "widget_timer_update()" is called, it > should change all timers from "on hold" to "active" so will get > processed from then on. Why shouldn't a new 'on hold' become 'active' in the same loop if 'widget_timer_update' is called multible times? And how would you guarantee that 'widget_timer_update' will ever be called again (to replace 'on hold' by 'active') if there is only one timer widget? Beside that I don't like the idea that third file functions which made use of timer.c & timer_group.c (might be many) have to make efforts to guarantee that they behave as exspected. But your basic idea might work. For timer_group.c (timer.c similar): You may replace 'widget' by a global (static) 'currentWidget' (initialized with -1 and set to -1 after the loop) in the process loop. In 'timer_add_widget' you set 'active' if (currentWidget<0 || widget<=currentWidget), else 'on hold'. If the process loop finds a 'on hold' it replaces it by 'active' and continues without processing this widget. If you've opened the files for changes already: I don't understand the realloc error handling in 'timer_add_widget' (similar in 'timer_add').In the case of a realloc error 'TimerGroupWidgets' was set to NULL. So there's no valid pointer to the existing TimerGroupWidgets after that. Won't that cause a memory leak and following errors cause nTimerGroupWidgets is only decreased by 1 instead of set it to 0? Shouldn't a additional pointer be used for the realloc return value to avoid this? Regards Marcus -- NEU: FreePhone 3-fach-Flat mit kostenlosem Smartphone! Jetzt informieren: http://mobile.1und1.de/?ac=OM.PW.PW003K20328T7073a |
|
From: Martin Z. <co...@mz...> - 2012-03-25 20:58:27
Attachments:
timer_linked_list.png
|
Hi Marcus! > I don't understand the realloc error handling in 'timer_add_widget' > (similar in 'timer_add').In the case of a realloc error 'TimerGroupWidgets' > was set to NULL. [...] I honestly just copied this from "timer.c". You're completely right, but being just faintly familiar with allocating and freeing memory in C (I mostly use higher programming languages), I hope that someone else chimes in. :) > Why shouldn't a new 'on hold' become 'active' in the same > loop if 'widget_timer_update' is called multible times? > And how would you guarantee that 'widget_timer_update' > will ever be called again (to replace 'on hold' by 'active') > if there is only one timer widget? While I don't understand the first part (probably some language barrier), your second point is valid. It was just a first idea to get things going. Still, I don't like the idea of designing a patch for the current problem (which could be as simple as changing "timer_add_widget" in "widget_timer.c" to "timer_add") and waiting for the problem to creep up again in another context. > Beside that I don't like the idea that third file functions > which made use of timer.c & timer_group.c (might be many) > have to make efforts to guarantee that they behave as exspected. Me neither. As I said, first idea... :) I do however quite like Mattias idea of using some sort of linked list. The attached image might be more clear than my words, but I'm thinking of this: each timer slot has a variable that points to the next timer slot (or to zero if it is the last one). There is an additional "main" pointer which points to the first timer slot (in red). On looping, the loop determines the first timer slot using the "main" pointer and then works its way through the linked list until it reaches a slot (the last one) whose pointer points to zero. If a timer slot is removed, the pointer of its predecessor is set to the removed slot's pointer. When a timer slot is added, it is added to the *beginning* of the list so it isn't processed prematurely. To make sure it will be processed in the next loop, the "main" pointer is set to the new slot. Already allocated memory would be determined and used just like before. Any comments? :) Martin -- www.mzuther.de www.radix-musik.de |
|
From: Michael R. <mi...@re...> - 2012-03-25 12:05:43
|
Hi Marcus, Martin, I try to think about the other stuff ASAP, but.... > I don't understand the realloc error handling in 'timer_add_widget' > (similar in 'timer_add').In the case of a realloc error 'TimerGroupWidgets' > was set to NULL. So there's no valid pointer to the existing > TimerGroupWidgets after that. Won't that cause a memory leak and > following errors cause nTimerGroupWidgets is only decreased by 1 > instead of set it to 0? Shouldn't a additional pointer be used for > the realloc return value to avoid this? You are *that* right. This is junk. junk junk junk. OTOH I think this will never happen in real life. When such a small allocation fails, I'm pretty sure that you will have other problems :-) anyway, this should be cleaned up. sunny greetings, Michael -- Michael Reinelt <mi...@re...> http://home.pages.at/reinelt GPG-Key 0xDF13BA50 ICQ #288386781 |
|
From: Mattia Jona-L. <mat...@gm...> - 2012-03-25 12:55:30
Attachments:
lcd4linux.diff
|
Hi everybody, I've been following this discussion about timers and I mostly agree with Martin when it comes to the implementation of timers in lcd4linux. As far as I can see there are two major drawbacks in the present implementation. First, all timers are stored in a malloc'ed vector that is reallocated every time a new slot is required. This is absolutely sub-optimal even when handling a handful of timers. The memory where slots are allocated is forced to be contiguous and there is no need for that. When no contiguous memory is available, realloc has to copy all pointers from the old location to the new one. Not to mention the fact that timers are added in a random order, filling the first available slot. This strategy requires the extra loop over all timers to find the next timer to be processed. The second drawback is that processed and unprocessed timers are kept in the same vector, triggering the bug reported by Marcus. What I propose is to switch to a linked list for timers and to create two separate linked lists, one for processed timers and one for unprocessed ones. The timers should be kept time ordered in the list and this will make the aforementioned extra loop not required. The processing routine will go through the active list and will process all timers one by one, unlinking them from the active list as soon as they are processed and linking them to the inactive list. Once the active list is empty, all timers will have been processed and are in the inactive list. Then it is sufficient to swap the active and inactive list and start again. In the meantime I attach a patch to fix the realloc error handling bug. Please feel free to comment on both! :) Mattia On Sun, Mar 25, 2012 at 2:05 PM, Michael Reinelt <mi...@re...> wrote: > Hi Marcus, Martin, > > I try to think about the other stuff ASAP, but.... > >> I don't understand the realloc error handling in 'timer_add_widget' >> (similar in 'timer_add').In the case of a realloc error 'TimerGroupWidgets' >> was set to NULL. So there's no valid pointer to the existing >> TimerGroupWidgets after that. Won't that cause a memory leak and >> following errors cause nTimerGroupWidgets is only decreased by 1 >> instead of set it to 0? Shouldn't a additional pointer be used for >> the realloc return value to avoid this? > > You are *that* right. This is junk. junk junk junk. > > OTOH I think this will never happen in real life. When such a small allocation fails, I'm pretty sure that you will have > other problems :-) > > anyway, this should be cleaned up. > > > > sunny greetings, Michael > > > -- > Michael Reinelt <mi...@re...> > http://home.pages.at/reinelt > GPG-Key 0xDF13BA50 > ICQ #288386781 > > ------------------------------------------------------------------------------ > This SF email is sponsosred by: > Try Windows Azure free for 90 days Click Here > http://p.sf.net/sfu/sfd2d-msazure > _______________________________________________ > Lcd4linux-devel mailing list > Lcd...@li... > https://lists.sourceforge.net/lists/listinfo/lcd4linux-devel |
|
From: Michael R. <mi...@re...> - 2012-03-25 13:57:41
|
Hi Mattia, nice to read you! again, I#ll comment just one part of your mail: > First, all timers are stored in a malloc'ed > vector that is reallocated every time a new slot is required. This is > absolutely sub-optimal even when handling a handful of timers. The > memory where slots are allocated is forced to be contiguous and there > is no need for that. When no contiguous memory is available, realloc > has to copy all pointers from the old location to the new one. Not to > mention the fact that timers are added in a random order, filling the > first available slot. I slightly disagree: normally there is a fixed number of timers used, and these timers are allocated at or very short after initialization of the layout. as soon as the maximum of timers are in use, no more realloc() should occur, even if timers are recreated (for one-shot timers). In case of a recreation, a already allocated but available slot will be reused. For all the discussions, please keep one aspect in mind: the "delay" value of every timer normally is a "property", that means that it is an expression, which is evaluated regularly, and its value may change during runtime. think of an expression like this: "is_working_hour() ? 50 : 2000" regards, Michael -- Michael Reinelt <mi...@re...> http://home.pages.at/reinelt GPG-Key 0xDF13BA50 ICQ #288386781 |
|
From: Michael R. <mi...@re...> - 2012-03-26 04:42:10
|
Hello Mattia, > In the meantime I attach a patch to fix the realloc error handling bug. applied and checked in, thanks a lot! -- Michael Reinelt <mi...@re...> http://home.pages.at/reinelt GPG-Key 0xDF13BA50 ICQ #288386781 |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X