-
Notifications
You must be signed in to change notification settings - Fork 879
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
feat(cherryusb): add cherryusb device & host demo #603
base: develop
Are you sure you want to change the base?
Conversation
#define FREERTOS_CONFIG_H | ||
|
||
// This example uses a common include to avoid repetition | ||
#include "FreeRTOSConfig_examples_common.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably just include https://github.com/raspberrypi/pico-examples/blob/develop/freertos/FreeRTOSConfig_examples_common.h rather than adding a separate copy of the file to the repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/raspberrypi/pico-examples/tree/master/freertos i use this template, freertos uses FreeRTOSConfig.h not FreeRTOSConfig_examples_common.h, so i don't know why add FreeRTOSConfig_examples_common.h.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe you could just do #include ../../../freertos/FreeRTOSConfig_examples_common.h
in your FreeRTOSConfig.h
? (although perhaps there's a "nicer" way of doing that without all the ../
?) And then you could remove FreeRTOSConfig_examples_common.h
from this PR. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using FreeRTOSConfig.h only? If configs in FreeRTOSConfig_examples_common.h have conflict with other demo like less memory, no feature support like sem & mutex disabled, users will be confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I just checked and I hadn't realised that FreeRTOSConfig_examples_common.h
doesn't use the normal
#ifndef SOME_SETTING
#define SOME_SETTING some_value
#endif
pattern. But @peterharperuk understands all of this much better than I do! 😆
@@ -0,0 +1,295 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this usb_config.h
is identical to the other usb_config.h
- perhaps it makes sense to add just one copy of the file to the repo (in usb/cherryusb/
?), and have both examples share it? 🤔
(i.e. like what is done with https://github.com/raspberrypi/pico-examples/blob/develop/freertos/FreeRTOSConfig_examples_common.h and https://github.com/raspberrypi/pico-examples/blob/develop/pico_w/wifi/lwipopts_examples_common.h and https://github.com/raspberrypi/pico-examples/blob/develop/pico_w/wifi/mbedtls_config_examples_common.h )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an optional config for users to modify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like enable log, enable bufsize for performance, enable more class instances and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, perhaps it makes sense to have a per-example usb_config.h
(where the users can add their optional customisation) which then does #include "usb_config_examples_common.h"
? (just to reduce the amount of duplication)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, perhaps it makes sense to have a per-example
usb_config.h
(where the users can add their optional customisation) which then does#include "usb_config_examples_common.h"
? (just to reduce the amount of duplication)
I know what you mean, yeah, reduce the duplication, also freertos. I will modify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have done. And i see pico_w demo has FreeRTOSConfig_examples_common.h
, so i move this into config dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have done.
Thanks 👍 But I'm still not sure if it makes sense to have both freertos/FreeRTOSConfig_examples_common.h
and usb/cherryusb/config/FreeRTOSConfig_examples_common.h
, which appear to be identical copies of each other?
In addition, current directory structure i think need to modify. |
cf5327c
to
4fd121d
Compare
Signed-off-by: sakumisu <1203593632@qq.com>
Signed-off-by: sakumisu <1203593632@qq.com>
raspberrypi/pico-sdk#2212