[GitHub] ccollins476ad commented on issue #984: hw/util/button: allow to specify eventq

[GitBox <gi...@apache.org>]

ccollins476ad commented on issue #984: hw/util/button: allow to specify eventq
URL: https://github.com/apache/mynewt-core/pull/984#issuecomment-378015498
 
 
   First, the code looks great.  I want to say that first, because this PR opens quite the can of worms, and I don't want my feedback to sound completely negative :).
   
   I get a little uncomfortable with specifying C code in syscfg settings (e.g., `os_eventq_dflt_get()`).  My worry is that this has the potential for some confusing debugging sessions in the case of a typo or syntax error.  There are also some thorny semantics issues, e.g., what if the code references the setting in an incorrect context (in an ISR, before the code is ready to run, etc.)?  I should acknowledge that there are already other syscfg settings that use C identifiers, so there is some precedent for this.
   
   This particular issue is something that has been lingering in my mind for a while. I think it would be good if the group decided on some form of project-wide policy, so I will kick off a discussion on the dev list.
   
   In the specific case of event queues, I believe all the other packages require the application to do the configuration at runtime.  For example, in the shell package: https://github.com/apache/mynewt-core/blob/2f35d09b6fa29e2b715d713d82015a1988bc87d6/sys/shell/src/shell.c#L48.  I am not sure that is necessarily "right", but it is how it is done elsewhere.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services