cfe2cos, the mega-merge #333
Conversation
… simple queue" This reverts commit 2d0a358.
There are known issues, but it should be fine for now
This commit also removes the redundant scheddev directory
|
I've addressed most of the above comments, will investigate the others. I also got rid of the cFE_emu library, and used the existing c_stubs mechanism. @gparmer In terms of review ordering, this order would be great:
Otherwise the original order of the summaries is a proxy for my desire to have that part reviewed. (Although, I think the filesystem should go last. It's already been reviewed after all.) Thanks! |
|
|
||
| int emu_backend_request_memory(spdid_t client); | ||
|
|
||
| int32 emu_CFE_ES_GetGenCount(spdid_t client); |
There was a problem hiding this comment.
You should use the cos_inv_token() in the server component for the client's spdid instead.
| ADDITIONAL_LIBS=-lcobj_format -lcos_kernel_api -lcos_defkernel_api -lsl_capmgr -lsl_sched -lheap -lsl_lock | ||
|
|
||
| include ../../Makefile.subsubdir | ||
| CFLAGS += -I./gen -I ./test/shared $(CPPFLAGS) |
There was a problem hiding this comment.
don't think there's supposed to be a space between -I .test.... I'm wondering what this test dir is. We'll see!
| #include <cos_component.h> | ||
| #include <cos_debug.h> | ||
| #include <cos_types.h> | ||
|
|
There was a problem hiding this comment.
You said you want feedback on style so: no spaces between many of these.
There was a problem hiding this comment.
We follow the std => cos => interface => cFE => internal header convention. Otherwise it becomes very confusing where each header is coming from. Especially since we have 5 header sources. Is this something the cos style guide takes a strong stance on?
| int | ||
| emu_backend_request_memory(spdid_t client) | ||
| { | ||
| vaddr_t our_addr = 0; |
There was a problem hiding this comment.
empty line after definitions
| int id = memmgr_shared_page_alloc(&our_addr); | ||
| assert(our_addr); | ||
| shared_regions[client] = (void *)our_addr; | ||
| return id; |
There was a problem hiding this comment.
blank line before final return.
| emu_CFE_EVS_Register(spdid_t client) | ||
| { | ||
| union shared_region *s = shared_regions[client]; | ||
| return CFE_EVS_Register(s->cfe_evs_register.filters, s->cfe_evs_register.NumEventFilters, |
There was a problem hiding this comment.
same thing about blank line after definitions. Won't repeat too many times, to keep the comment # down.
| cycles_t now, start; | ||
|
|
||
| rdtscll(start); | ||
| start += sl_usec2cyc(HZ_PAUSE); |
There was a problem hiding this comment.
are we uniformly 64 bit here? If not, could overflow easily.
|
|
||
| while (1) { | ||
| rdtscll(now); | ||
| if (now > start) { |
There was a problem hiding this comment.
start is not a good name for the variable being used here. Would use deadline as deadline = start + sl_usec2cyc(HZ_PAUSE).
We need to think through how to have asserts for all of the overflow possibilities everywhere. I'm concerned this is why your scheduling is wonky.
| void | ||
| OS_SchedulerStart(cos_thd_fn_t main_delegate) | ||
| { | ||
| sl_init(SL_MIN_PERIOD_US); |
| struct sl_thd * main_delegate_thread = sl_thd_alloc(main_delegate, NULL); | ||
| union sched_param_union sp = {.c = {.type = SCHEDP_PRIO, .value = MAIN_DELEGATE_THREAD_PRIORITY}}; | ||
| sl_thd_param_set(main_delegate_thread, sp.v); | ||
| main_delegate_thread_id = sl_thd_thdid(main_delegate_thread); |
There was a problem hiding this comment.
"delegate thread" is a little bit of a strange name. Not really sure what "delegate" has to do with all of this.
There was a problem hiding this comment.
It's called main_delegate_thread. It's the thread we delegate main execution to. Do you have a better name in mind?
There was a problem hiding this comment.
I read it as, "of all the delegate threads, it is the main one". Both grammatical parsings are correct.
Maybe just a comment somewhere when it is first used to clarify.
| it. */ | ||
| } | ||
|
|
||
| sl_thd_block_periodic(0); |
There was a problem hiding this comment.
just to make sure we aren't spinning in this loop for some strange interaction in the system, it might be smart to count how many times we've iterated without triggering the now > start condition, and assert if it goes above some level (say 64).
| struct sl_thd * main_delegate_thread = sl_thd_alloc(main_delegate, NULL); | ||
| union sched_param_union sp = {.c = {.type = SCHEDP_PRIO, .value = MAIN_DELEGATE_THREAD_PRIORITY}}; | ||
| sl_thd_param_set(main_delegate_thread, sp.v); | ||
| main_delegate_thread_id = sl_thd_thdid(main_delegate_thread); |
There was a problem hiding this comment.
I read it as, "of all the delegate threads, it is the main one". Both grammatical parsings are correct.
Maybe just a comment somewhere when it is first used to clarify.
| task_info->osal_task_prop.priority = MAIN_DELEGATE_THREAD_PRIORITY; | ||
| task_info->osal_task_prop.OStask_id = (uint32)sl_thd_thdid(main_delegate_thread); | ||
|
|
||
| struct sl_thd * timer_thd = sl_thd_alloc(timer_fn_1hz, NULL); |
| void | ||
| osal_task_entry_wrapper(void *task_entry) | ||
| { | ||
| ((osal_task_entry)task_entry)(); |
There was a problem hiding this comment.
if we define this type, it should be osal_task_entry_t.
There was a problem hiding this comment.
Unfortunately, this is a cFE definition
|
|
||
| if (priority > 255 || priority < 1) { return OS_ERR_INVALID_PRIORITY; } | ||
|
|
||
| struct sl_thd *thd = sl_thd_alloc(osal_task_entry_wrapper, function_pointer); |
| union sched_param_union sp = {.c = {.type = SCHEDP_PRIO, .value = priority}}; | ||
| sl_thd_param_set(thd, sp.v); | ||
|
|
||
| struct cfe_task_info *task_info = &cfe_tasks[sl_thd_thdid(thd)]; |
There was a problem hiding this comment.
ha, our thdid function has a pretty redundant name.
| OS_MutSemDelete(uint32 sem_id) | ||
| { | ||
| int32 result = OS_SUCCESS; | ||
| sl_lock_take(&mutex_data_lock); |
|
|
||
| sl_lock_take(&mutex_data_lock); | ||
|
|
||
| if (sem_id == NULL || sem_name == NULL) { |
There was a problem hiding this comment.
move outside of lock along with next error check
| } | ||
| } | ||
|
|
||
| /* The name was not found in the table, |
|
|
||
| sl_lock_take(&semaphore_data_lock); | ||
|
|
||
| if (sem_id == NULL || sem_name == NULL) { |
There was a problem hiding this comment.
move these error checks out of the lock
There was a problem hiding this comment.
since these checks are repetitive, you might want to move them into a function.
| OS_SemaphoreDelete(struct semaphore *semaphores, uint32 max_semaphores, uint32 sem_id) | ||
| { | ||
| int32 result = OS_SUCCESS; | ||
| sl_lock_take(&semaphore_data_lock); |
Brought five new apps into the cFE repo. They each run as their own component. mm is current not working, requiring more stubs to be written. All of the infrastructure is there from the cFE side of things, but it is excluded from the make.py list of apps.
This includes a critical hypercall to advance intialization
Summary of this Pull Request (PR)
The cFE is a complex piece of software, consequently our support implementation is quite complex. I will attempt to summarize what is going on with each change.
cFE_booter Support Code
cos_initto the actual cFE is a bit of code, most of which is inspired by the posix implementation. We need to set up all of our data structures, start our scheduler, and make sure the filesystem is in good order. It's spread across of few files -- but again, not too complicated. We also boost the stack size so that the cFE doesn't blow the small stack. You can trace configuration logic fromcFE_entrypoint.cThe only other interesting stuff is incFE_util.c.cFE_booter/cFE_entrypoint.ccFE_booter/cFE_util.ccFE_booter/cFE_util.hparts ofcFE_booter/osapi.candcFE_booter/ostask.cinclude/shared/consts.hGeneral OSAL Method Implementations
cFE_booter/osapi.ccFE_booter/psp.ccFE_booter/ostimer.ccFE_booter/osnetwork.cCopied Files
cFE_booter/cfe_psp_*App Loading
cFE_booter/osloader.ccFE Tasks + Scheduling
cFE_booter/ostask.ccFE_booter/ostask.hcFE_booter/sl_mod_fprr.ccFE_booter/sl_mod_policy.hcFE_booter/sl_thd_static_backend.cOSAL Filesystem APIS
cFE_booter/osfiles.ccFE_booter/osfilesys.ccFE_booter/osfilesys.hcFE_booter/tar.ccFE_booter/tar.hQueue Implementation
cFE_booter/osqueue.cMajor Build System Changes
makewrangling. We kept most of our original build logic--in python--but it now runs totally under composite'smake. I think most of the custom stuff is well commented..gitignore.gitmodulessrc/Makefileextern/make.pymakefoo is weak, so some of this is a bit of a cludge. I just don't know how to do it better.cFE App Component Wrappers
no_interface/sch_lab/*no_interface/sample_lib/*no_interface/sample_app/*cFE emulation for apps
src/components/Makefile.compcFE_booter/cFE_emu_support.clib/cFE_emu.clib/Makefileinclude/cFE_emu.hcFE_booter Build System
cFE_booter/.gitignorecFE_booter/MakefileRunning the cFE
tools/Vagrantfilerunscripts/cFE_booter.shrunscripts/llboot_cFE.shOther Outstanding Issues
llbooter/boot_deps.handllbooter/llbooter.care modified to map an extra page per component. This sucks, but without it the cFE breaks.include/cos_asm_simple_stacks.h) This is because their composite versions are utterly broken and don't respectconsts.h. I don't implement the most elegant solution, but w/e.Intent for your PR
Choose one (Mandatory):
Reviewers (Mandatory):
(Specify @<github.com username(s)> of the reviewers. Ex: @user1, @user2)
@gparmer @base0x10 @phanikishoreg
Code Quality
As part of this pull request, I've considered the following:
Style:
Code Craftsmanship:
Testing
I've tested the code using the following test programs (provide list here):