Remove global structure in LCD HAL Layer #6597
Conversation
abhinav-s235
changed the title
os/drivers/lcd/lcd_dev.c
Outdated
|
|
||
| int pid = kernel_thread("LCD Frame flusing", 204, 8192, lcd_flushing_thread, NULL); | ||
| itoa((int)lcd_info, parm_buf, 16); | ||
| parm[0] = parm_buf; |
There was a problem hiding this comment.
param[0] = itoa((int)lcd_info, parm_buf, 16);
And don't we need to memset parm_buf?
There was a problem hiding this comment.
I don't know why we need to convert it to ascii and to reconvert to integer.
Is it possible to give the address itself without converting?
There was a problem hiding this comment.
It is not possible to pass the address without converting it to string because kernel_thread() function accept the parameter of type FAR char *const argv[].
int kernel_thread(FAR const char *name, int priority, int stack_size, main_t entry, FAR char *const argv[])
There was a problem hiding this comment.
param[0] = itoa((int)lcd_info, parm_buf, 16);And don't we need to memset parm_buf?
I don't think it is needed
could you please explain why you think so?
os/drivers/lcd/lcd_dev.c
Outdated
| char *parm[2]; | ||
| char parm_buf[9]; /* for storing 32 bit address */ |
There was a problem hiding this comment.
The param and param_buf are difficult to distinguish and to know what they are. Could you rename them to know what they are in name?
There was a problem hiding this comment.
The param and param_buf are difficult to distinguish and to know what they are. Could you rename them to know what they are in name?
fixed
os/drivers/lcd/lcd_dev.c
Outdated
|
|
||
| int pid = kernel_thread("LCD Frame flusing", 204, 8192, lcd_flushing_thread, NULL); | ||
| itoa((int)lcd_info, parm_buf, 16); | ||
| parm[0] = parm_buf; |
There was a problem hiding this comment.
I don't know why we need to convert it to ascii and to reconvert to integer.
Is it possible to give the address itself without converting?
…de lcddev_register function Replaced the global LCD HAL layer instance with a local declaration within the lcddev_register function and passed this instance to LCD_FLUSH_THREAD by converting its address to a string using atoi() function.
abhinav-s235
force-pushed
the
lcd_flushing_thread
branch
from
3d5064e to
0229ed0
Compare
@abhinav-s235 Could you check my comment above? I mean why you did below |
Previously, while creating frame flushing thread we were not passing lcd_info pointer to function lcd_flushing_thread() that's why we declared it globally but now we are passing it as a parameter to the lcd_flushing_thread() function there is no need to globally declare it. |
|
|
||
| int pid = kernel_thread("LCD Frame flusing", 204, 8192, lcd_flushing_thread, NULL); | ||
| itoa((int)lcd_info, lcd_info_addr, 16); | ||
| task_info[0] = lcd_info_addr; |
There was a problem hiding this comment.
The lcd_info_addr is a local pointer variable. Is it ok in lifecycle? When the flushing thread gets the arg, is it still valid?
There was a problem hiding this comment.
@sunghan-chang - Yes, it can happen that thread executes after scope of lcd_info_addr is finished.
But to encounter this, if we use dynamic(malloc) lcd_info_addr, in thread function, the passed argument's address changes so we can't free it.
So should we keep static struct *lcd_info as global only ?
| @@ -423,13 +424,15 @@ int lcddev_register(struct lcd_dev_s *dev) | |||
| { | |||
| char devname[16] = { 0, }; | |||
| int ret; | |||
| char *task_info[2]; | |||
There was a problem hiding this comment.
How about flushing_thread_args?
The point is you want to remove the global variable. Right? Is that to reduce memory usages? |
Replaced the global LCD HAL layer instance with a local declaration within the lcddev_register() function and passed this instance to LCD_FLUSH_THREAD by converting its address to a string using atoi() function.