Input_SRV: migration to event_loop #3968
Conversation
|
Compiled f7 firmware for commit |
Skorpionm
requested review from
skotopes,
DrZlo13,
hedger and
gsurkov
as code owners
| input_semaphore_callback, | ||
| instance); | ||
|
|
||
| instance->key_sequence = malloc(sizeof(InputSrvKeySequence) * input_pins_count); |
There was a problem hiding this comment.
All those allocations could easily land on the stack, VLAs come in handy here (copying my remark from the previous PR).
There was a problem hiding this comment.
no, if you put everything on the stack and turn on debugging, then the 1 KB stack allocated to the process is sometimes not enough and everything crashes with special effects
There was a problem hiding this comment.
Those structures are tiny - so wouldn't this mean that even without them on the stack, 1KB is pushing the limit very close?
There was a problem hiding this comment.
all structures on the stack will take about 220 bytes.
provided that there are only 6 buttons, when the number of buttons changes, the size of the structure will increase and if the structure is placed on the stack, the implementation ceases to be universal.
There was a problem hiding this comment.
at the expense of 1kb on the stack it is enough even with debugging enabled, without it about 500 bytes are used per process
There was a problem hiding this comment.
In this case the keys could stay on the heap (although maybe put them on the service heap?) while instance goes to the stack? It does not have to be all or nothing.
There was a problem hiding this comment.
Well then what's the point of chopping it into pieces?
There was a problem hiding this comment.
Currently it's chopped into pieces already, and occupies 3 heap allocations - so by moving you save one allocation, which is better than nothing.
There was a problem hiding this comment.
Keep in mind that services are allocated in special memory region and it's quite small. Increasing stack size may bring another set of problems
There was a problem hiding this comment.
Sure, in this case the key arrays could stay on the user heap, but the inputsrv is so small there's no reason not to put it on the stack. Before this PR it was just a bunch of local variables.
| furi_record_create(RECORD_INPUT_EVENTS, event_pubsub); | ||
| static void input_isr_key(void* context) { | ||
| InputSrv* instance = context; | ||
| furi_semaphore_release(instance->input_semaphore); |
There was a problem hiding this comment.
Interrupts may be missed in some edge cases
What's new
Verification
Checklist (For Reviewer)