Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Expose the static task function #204

Closed

Conversation

Jerrylum
Copy link
Contributor

@Jerrylum Jerrylum commented Jan 25, 2020

Summary:

I was looking at issue #168. Expose the static task function can help us get a foot in the door.

Motivation:

Expose it and 90% of users don't even know what is it.

References (optional):

#168

Test Plan:

Tested 957603f

  • run test code mutexes.c
  • run test code semaphores.c
  • run test code static_tast_states.c

@Jerrylum Jerrylum changed the title ✨ Expose the static task function ✨ Expose the static task function Jan 25, 2020
Makefile Outdated Show resolved Hide resolved
include/pros/apix.h Outdated Show resolved Hide resolved
include/pros/apix.h Show resolved Hide resolved
Copy link
Member

@nathan-moore nathan-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple notes on wrong headers and stuff I don't think we should expose. Your test also only covers tasks, but it really should have 100% coverage over what you're adding. You can also consider adding it/augmenting some of the tests in the test folder as there are some related.

On another note related to decoupling lvgl, this isn't strictly necessary for that. You likely could just use tasks on the heap and it would work fine. I'm not opposed to taking this route though, as there are likely uses for this that I don't know about.

Comment on lines 56 to 71
#define portUSING_MPU_WRAPPERS 0
#define configMAX_TASK_NAME_LEN ( 32 )
#define portSTACK_GROWTH ( -1 )
#define configRECORD_STACK_HIGH_ADDRESS 0
#define portCRITICAL_NESTING_IN_TCB 0
#define configUSE_TRACE_FACILITY 1
#define configUSE_MUTEXES 1
#define configUSE_APPLICATION_TASK_TAG 0
#define configNUM_THREAD_LOCAL_STORAGE_POINTERS 2
#define configGENERATE_RUN_TIME_STATS 1
#define configUSE_NEWLIB_REENTRANT 1
#define configUSE_TASK_NOTIFICATIONS 1
#define configSUPPORT_STATIC_ALLOCATION 1
#define configSUPPORT_DYNAMIC_ALLOCATION 1
#define INCLUDE_xTaskAbortDelay 1
#define configUSE_QUEUE_SETS 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these should be exposed to the end user. I think manually evaluating the macros would be best. Also if we change these in the future, it would be a binary breaking change. I don't think it's a big issue, but it's worth mentioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to create static_task_s_t, I evaluated the final value for the macros here. Is that what manually evaluating mean or do you what to remove all config macro to the end-user?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, my thoughts are that these values are implementation details of FreeRTOS, it's micro-controller specific port, and how we've configured it. They really shouldn't be available outside the kernel. In order for that to happen, then, yes, all the config macros will effectively have to be removed in this header. It isn't the best maintenance wise though, so I'm open to other approaches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I removed all the config macros.

include/pros/apix.h Outdated Show resolved Hide resolved
include/pros/apix.h Show resolved Hide resolved
include/pros/apix.h Outdated Show resolved Hide resolved
include/pros/apix.h Outdated Show resolved Hide resolved
@Jerrylum
Copy link
Contributor Author

A couple notes on wrong headers and stuff I don't think we should expose. Your test also only covers tasks, but it really should have 100% coverage over what you're adding. You can also consider adding it/augmenting some of the tests in the test folder as there are some related.

On another note related to decoupling lvgl, this isn't strictly necessary for that. You likely could just use tasks on the heap and it would work fine. I'm not opposed to taking this route though, as there are likely uses for this that I don't know about.

Yes, I will change my test code since it is not suitable after commit 5d10690.

@HotelCalifornia
Copy link
Contributor

The more I think about this, the more conflicted I get. I know I had mentioned wanting to expose static tasks so that we could more easily decouple LVGL from the kernel, but now I don't think we really even need them---a normal task would probably work just as well in this case.

Sorry to bring this up after you've done so much work on it already...

@nathan-moore
Copy link
Member

I agree with Alex. That, and with how tightly coupled static tasks are with FreeRTOS, I don't really like exposing them. Thanks for looking into it though. Do you have an opinion otherwise Jerrylum, or should this be closed?

@Jerrylum
Copy link
Contributor Author

👌

@Jerrylum Jerrylum closed this Feb 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants