[controller] add schedule migration API #2435
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2435 +/- ##
===========================================
- Coverage 55.77% 45.50% -10.28%
===========================================
Files 87 100 +13
Lines 6890 11311 +4421
Branches 0 815 +815
===========================================
+ Hits 3843 5147 +1304
- Misses 3047 5909 +2862
- Partials 0 255 +255 ☔ View full report in Codecov by Sentry. |
Irving-cl
force-pushed
the
add_schedule_migrate_api
branch
2 times, most recently
from
5e4ac60
to
b746aca
Compare
Irving-cl
force-pushed
the
add_schedule_migrate_api
branch
from
b746aca
to
a86890e
Compare
src/ncp/ncp_spinel.cpp
Outdated
| void NcpSpinel::DatasetMgmtSetPending(const otOperationalDatasetTlvs &aPendingOpDatasetTlvs, AsyncTaskPtr aAsyncTask) | ||
| { | ||
| otError error = OT_ERROR_NONE; | ||
| EncodingFunc encodingFunc = [this, &aPendingOpDatasetTlvs] { |
There was a problem hiding this comment.
How can you ensure aPendingOpDatasetTlvs is always alive? IIUC it could be a local variable in DBusThreadObjectNcp::ScheduleMigrationHandler.
There was a problem hiding this comment.
The call stack is: DBus API -> ThreadHost API -> NcpSpinel API -> encodingFunc(pendingDataset)
And the DBus API will only exit after calling the whole stack, including the encoding function. So the variable is still valid.
I agree it's a little confusing here. Here an encoding func is used in SetProperty NOT for async purposes but just for reusing the code of setting properties (get a tid, check if there is an available tid, encode a header, then encode the body, send the frame, etc). Another approach would be using templates: we templatize SetProperty and use template specialization for encoding the body. But I think that leads to an increasing of code size. Thoughts?
There was a problem hiding this comment.
IMHO we may consider passing in aPendingOpDatasetTlvs by value or storing it into a shared_ptr and pass it in by value so that we can feel safer when reviewing/updating the code.
src/ncp/ncp_spinel.cpp
Outdated
| void NcpSpinel::DatasetMgmtSetPending(const otOperationalDatasetTlvs &aPendingOpDatasetTlvs, AsyncTaskPtr aAsyncTask) | ||
| { | ||
| otError error = OT_ERROR_NONE; | ||
| EncodingFunc encodingFunc = [this, &aPendingOpDatasetTlvs] { |
Irving-cl
force-pushed
the
add_schedule_migrate_api
branch
from
a86890e
to
c7499d8
Compare
Irving-cl
force-pushed
the
add_schedule_migrate_api
branch
2 times, most recently
from
60ecbc0
to
76f2882
Compare
Irving-cl
force-pushed
the
add_schedule_migrate_api
branch
from
76f2882
to
0b4882f
Compare
This PR adds the
ScheduleMigrationAPI toThreadHostas well as the implementation for NcpHost.The PR also replaces the empty implementation in DBusNcp with the real implementation by calling the API. The PR adds an expect test to verify the result of a migration.