Conversation
Bring up libcsp for FINCH. Add the finch_csp_init() API, start the router thread, and bind the built-in CSP service handlers. This does not add an external transport yet. Signed-off-by: Mirai SHINJO <oss@mshinjo.com>
Enable FINCH CSP for OBC. Set the node address and hostname, and initialize CSP at app startup. Signed-off-by: Mirai SHINJO <oss@mshinjo.com>
Enable FINCH CSP for PAY. Set the node address and hostname, and initialize CSP at app startup. Signed-off-by: Mirai SHINJO <oss@mshinjo.com>
mshinjo
requested review from
TheArchons,
alexapostolu,
chopikus,
coby1kenobi and
swarnavaghosh04
as code owners
| K_NO_WAIT); | ||
|
|
||
| if (finch_csp_router_tid == NULL) { | ||
| LOG_ERR("Failed to creat CSP router thread"); |
| ARG_UNUSED(arg3); | ||
|
|
||
| while (1) { | ||
| (void)csp_route_work(); |
There was a problem hiding this comment.
Why do we ignore the return value here? Do we want to log any errors csp_route_work returns?
|
|
||
| config FINCH_CSP_ROUTER_PRIORITY | ||
| int "CSP router thread priority" | ||
| default 0 |
There was a problem hiding this comment.
What is the reasoning for priority to default to 0? I am not very familiar with threading, but from what I can tell, this controls the priority for finch_csp_router_thread. If priority is zero, then it has the same priority as the main loop (0 by default). Since finch_csp_router_thread has an infinite loop, would we risk being stuck looping through this thread indefinitely and not executing the rest of main?
| k_mutex_lock(&finch_csp_lock, K_FOREVER); | ||
|
|
||
| if (finch_csp_initialized) { | ||
| goto out; |
There was a problem hiding this comment.
Should we add a log here to say something like "finch csp already initialized, skipping initialization"?
|
|
||
| ret = finch_csp_bind_services(); | ||
| if (ret < 0) { | ||
| goto out; |
There was a problem hiding this comment.
A LOG_ERR here as well might be useful
| int ret = finch_csp_init(); | ||
|
|
||
| if (ret < 0) { | ||
| LOG_ERR("Failed to initialize FINCH CSP (%d)", ret); |
There was a problem hiding this comment.
We should think of a way to recover from this if libcsp fails. We especially don't want any failures in main initialization
But this can be done in another PR
| k_mutex_lock(&finch_csp_lock, K_FOREVER); | ||
|
|
||
| if (finch_csp_initialized) { | ||
| goto out; |
There was a problem hiding this comment.
unlock and return is better than goto I think
There was a problem hiding this comment.
Why do you think it's better? I think goto works better here because we're repeating less code. If we ever want to do anything else at the end of the function (like more cleanup) we can just add it to after out, rather than needing to update all returns, which there's a good chance we forget to update all of them.
The goto isn't that bad here because it's jumping further down the code, we aren't writing loops with a goto. But if you want to avoid gotos, would it be better to extract the main body of the function to another function and have this function just do the lock, call the other function, then unlock?
3 participants
Bring up libcsp for FINCH and enable FINCH_CSP in OBC and PAY apps.
Add the finch_csp_init() API, start the router thread,
and bind the built-in CSP service handlers.
This does not add an external transport yet.