Skip to content

progress towards decoupling cec and cdc functionality, et-al #51

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

Merged
merged 3 commits into from
Jul 13, 2025

Conversation

Inspirati
Copy link
Contributor

decouple cec user control name strings from both config and log modules

  • allows cec-log to exist independent of the application configuration (cec-config) functionality
  • cec-log could readily be minimised to not support higher level cec user control specific support
  • cec-config need know nothing else about the cec logging capability (this may warrant a re-think)
  • cec-config just needs access to a string table which it doesn't make sense to own

move and rename function cec_get_uptime_ms() to cec_log_uptime_ms()

  • usb-cdc is already a client of cec-log
  • function is not referenced in cec-task
  • as for the name, it really isn't even cec specific in any way, but is the timebase for log entries
  • usb-cdc only uses it to display stats on the console where the log outputs
  • perhaps it should migrate to a lower platform specific layer, as its implementation specific anyhow, we just don't have that layer defined as yet

decouple cec-log from usb-cdc by initialising with pointer to the log function

  • cec-log need not have anything to do with the usb layer(s)

also sneak a few other dependency (.h inclusion) reductions in

todo: cec-config contains code which is foreign from pure cec requirements

todo: recently introduced include/config.h may somehow conflict/override with pico-sdk having same

todo: address issue of _CDC_BR being the only thing left requiring inclusion of usb-cdc.h in certain cec modules

Inspirati and others added 2 commits July 6, 2025 20:50
decouple cec user control name strings from both config and log modules
- allows cec-log to exist independent of the application configuration (cec-config) functionality
- cec-log could readily be minimised to not support higher level cec user control specific support
- cec-config need know nothing else about the cec logging capability (this may warrant a re-think)
- cec-config just needs access to a string table which it doesn't make sense to own

move and rename function cec_get_uptime_ms() to cec_log_uptime_ms()
- usb-cdc is already a client of cec-log
- function is not referenced in cec-task
- as for the name, it really isn't even cec specific in any way, but is the timebase for log entries
- usb-cdc only uses it to display stats on the console where the log outputs
- perhaps it should migrate to a lower platform specific layer, as its implementation specific anyhow, we just don't have that layer defined as yet

decouple cec-log from usb-cdc by initialising with pointer to the log function
- cec-log need not have anything to do with the usb layer(s)

also sneak a few other dependency (.h inclusion) reductions in

todo: cec-config contains code which is foreign from pure cec requirements

todo: recently introduced include/config.h may somehow conflict/override with pico-sdk having same

todo: address issue of _CDC_BR being the only thing left requiring inclusion of usb-cdc.h in cec modules
Move 'config.h' to 'pico-cec/config.h' as an initial attempt to
namespace the header.
Add the 'util' module to house very generic symbols (eg. uptime).
@gkoh
Copy link
Owner

gkoh commented Jul 8, 2025

@Inspirati Overall, refactor looks good.
I hope you don't mind if I commit to your branch.

I've addressed the config.h issue by moving it to pico-cec/config.h, otherwise it will eventually collide in a surprising way one day.
Perhaps you have a better idea.

Furthermore, I moved the uptime symbol into its own module.
It could also be called port_* ala FreeRTOS naming style and could become the platform specific adapter layer for:

  • uptime
  • timer manipulation
  • isr handling?
  • log printing?
    • not functionally different to the callback in this PR

I have not yet tested this on hardware.

Add _LOG_BR macro for _CDC_BR to streamline header inclusion.
Any modules requiring logging need only include `cec-log.h`.
@gkoh
Copy link
Owner

gkoh commented Jul 13, 2025

todo: recently introduced include/config.h may somehow conflict/override with pico-sdk having same

Introduced include/pico-cec/config.h to attempt a basic header namespace.
This requires calling modules to #include "pico-cec/config.h" and does avoid the collision with every other config.h out there.

todo: address issue of _CDC_BR being the only thing left requiring inclusion of usb-cdc.h in certain cec modules

Added _LOG_BR, which just maps to _CDC_BR, narrowing the inclusion dependency tree.

I have done a run-time test and things are still working as before.
So, merging it, thanks @Inspirati!

@gkoh gkoh merged commit 9110912 into gkoh:main Jul 13, 2025
7 checks passed
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.

2 participants