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.

Sign up for GitHub

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

Jump to bottom

Adding support for a generic SOF callback #2213

Merged
merged 16 commits into from
May 10, 2024
Merged

Adding support for a generic SOF callback #2213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0161955
Adding support for a SOF callback
Rocky04 Aug 8, 2023
a98b219
Remove port from SOF callback
Rocky04 Aug 8, 2023
427ecbb
Bad line break at start
Rocky04 Aug 8, 2023
997c29b
SOF ISR control
Rocky04 Sep 14, 2023
288f24b
Missing semicolon
Rocky04 Sep 14, 2023
46977a0
Fixign errors
Rocky04 Sep 14, 2023
c87fba1
Bug
Rocky04 Sep 14, 2023
337d03d
Incorrect comment
Rocky04 Sep 14, 2023
6c286e3
Merge branch 'master' into patch-5
HiFiPhile May 9, 2024
c8beaad
Switch to weak default implementation.
HiFiPhile May 9, 2024
11b5b2a
Move sof status into usbd_device_t.
HiFiPhile May 9, 2024
ca479d6
Disable SOF on configuration change.
HiFiPhile May 9, 2024
16cd92f
Don't forget to queue SOF event.
HiFiPhile May 9, 2024
376b439
Convert to bit-field since it's more reliable.
HiFiPhile May 9, 2024
eea7d7b
Fix CI.
HiFiPhile May 9, 2024
36ba42c
Little optimization.
HiFiPhile May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/class/audio/audio_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1835,7 +1835,7 @@ static bool audiod_set_interface(uint8_t rhport, tusb_control_request_t const *
audio->feedback.frame_shift = desc_ep->bInterval -1;

// Enable SOF interrupt if callback is implemented
if (tud_audio_feedback_interval_isr) usbd_sof_enable(rhport, true);
if (tud_audio_feedback_interval_isr) usbd_sof_enable(rhport, SOF_CONSUMER_AUDIO, true);
}
#endif
#endif // CFG_TUD_AUDIO_ENABLE_EP_OUT
Expand Down Expand Up @@ -1909,7 +1909,7 @@ static bool audiod_set_interface(uint8_t rhport, tusb_control_request_t const *
break;
}
}
if (disable) usbd_sof_enable(rhport, false);
if (disable) usbd_sof_enable(rhport, SOF_CONSUMER_AUDIO, false);
#endif

#if CFG_TUD_AUDIO_ENABLE_EP_IN && CFG_TUD_AUDIO_EP_IN_FLOW_CONTROL
Expand Down
57 changes: 45 additions & 12 deletions src/device/usbd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ TU_ATTR_WEAK void tud_event_hook_cb(uint8_t rhport, uint32_t eventid, bool in_is
(void)in_isr;
}

TU_ATTR_WEAK void tud_sof_cb(uint32_t frame_count) {
(void)frame_count;
}
Comment on lines +59 to +61
Copy link
Contributor Author

@Rocky04 Rocky04 May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why not have a weak declaration in the header file? After all it's used only once at #L629 anyway. So a check there for a definition similar how it's done for other things should be more inline.

Edit:
In that case there can be a sanity check. Like to return false at #L394 if there is no function definition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are converting to weak default implementation for keil and clang compatibility.


//--------------------------------------------------------------------+
// Device Data
//--------------------------------------------------------------------+
Expand All @@ -76,6 +80,7 @@ typedef struct {
volatile uint8_t cfg_num; // current active configuration (0x00 is not configured)
uint8_t speed;
volatile uint8_t setup_count;
volatile uint8_t sof_consumer;

uint8_t itf2drv[CFG_TUD_INTERFACE_MAX]; // map interface number to driver (0xff is invalid)
uint8_t ep2drv[CFG_TUD_ENDPPOINT_MAX][2]; // map endpoint to driver ( 0xff is invalid ), can use only 4-bit each
Expand Down Expand Up @@ -275,6 +280,7 @@ TU_ATTR_ALWAYS_INLINE static inline usbd_class_driver_t const * get_driver(uint8
return driver;
}


//--------------------------------------------------------------------+
// DCD Event
//--------------------------------------------------------------------+
Expand Down Expand Up @@ -382,6 +388,12 @@ bool tud_connect(void) {
return true;
}

bool tud_sof_cb_enable(bool en)
{
usbd_sof_enable(_usbd_rhport, SOF_CONSUMER_USER, en);
return true;
Copy link
Contributor Author

@Rocky04 Rocky04 May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A return value makes no sense here.

While also true for my original PR that wasn't as obvious. I wasn't sure if every driver has implemented dcd_sof_enable() and so used TU_VERIFY() to check for that.

It would make sense if there is a check if an implementation of a weak declared tud_sof_cb() is present.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right, forgot to change.

}

//--------------------------------------------------------------------+
// USBD Task
//--------------------------------------------------------------------+
Expand Down Expand Up @@ -612,6 +624,12 @@ void tud_task_ext(uint32_t timeout_ms, bool in_isr) {
break;

case DCD_EVENT_SOF:
if (tu_bit_test(_usbd_dev.sof_consumer, SOF_CONSUMER_USER)) {
TU_LOG_USBD("\r\n");
tud_sof_cb(event.sof.frame_count);
}
break;

default:
TU_BREAKPOINT();
break;
Expand Down Expand Up @@ -702,6 +720,9 @@ static bool process_control_request(uint8_t rhport, tusb_control_request_t const
// already configured: need to clear all endpoints and driver first
TU_LOG_USBD(" Clear current Configuration (%u) before switching\r\n", _usbd_dev.cfg_num);

// disable SOF
dcd_sof_enable(rhport, false);

// close all non-control endpoints, cancel all pending transfers if any
dcd_edpt_close_all(rhport);

Expand Down Expand Up @@ -1101,6 +1122,14 @@ TU_ATTR_FAST_FUNC void dcd_event_handler(dcd_event_t const* event, bool in_isr)
break;

case DCD_EVENT_SOF:
// SOF driver handler in ISR context
for (uint8_t i = 0; i < TOTAL_DRIVER_COUNT; i++) {
usbd_class_driver_t const* driver = get_driver(i);
if (driver && driver->sof) {
driver->sof(event->rhport, event->sof.frame_count);
}
}

// Some MCUs after running dcd_remote_wakeup() does not have way to detect the end of remote wakeup
// which last 1-15 ms. DCD can use SOF as a clear indicator that bus is back to operational
if (_usbd_dev.suspended) {
Expand All @@ -1110,15 +1139,10 @@ TU_ATTR_FAST_FUNC void dcd_event_handler(dcd_event_t const* event, bool in_isr)
queue_event(&event_resume, in_isr);
}

// SOF driver handler in ISR context
for (uint8_t i = 0; i < TOTAL_DRIVER_COUNT; i++) {
usbd_class_driver_t const* driver = get_driver(i);
if (driver && driver->sof) {
driver->sof(event->rhport, event->sof.frame_count);
}
if (tu_bit_test(_usbd_dev.sof_consumer, SOF_CONSUMER_USER)) {
dcd_event_t const event_sof = {.rhport = event->rhport, .event_id = DCD_EVENT_SOF};
queue_event(&event_sof, in_isr);
}

// skip osal queue for SOF in usbd task
break;

case DCD_EVENT_SETUP_RECEIVED:
Expand Down Expand Up @@ -1355,12 +1379,21 @@ void usbd_edpt_close(uint8_t rhport, uint8_t ep_addr) {
return;
}

void usbd_sof_enable(uint8_t rhport, bool en) {
void usbd_sof_enable(uint8_t rhport, sof_consumer_t consumer, bool en) {
rhport = _usbd_rhport;

// TODO: Check needed if all drivers including the user sof_cb does not need an active SOF ISR any more.
// Only if all drivers switched off SOF calls the SOF interrupt may be disabled
dcd_sof_enable(rhport, en);
uint8_t consumer_old = _usbd_dev.sof_consumer;
// Keep track how many class instances need the SOF interrupt
if (en) {
_usbd_dev.sof_consumer |= (uint8_t)(1 << consumer);
} else {
_usbd_dev.sof_consumer &= (uint8_t)(~(1 << consumer));
}

// Test logically unequal
Copy link
Contributor Author

@Rocky04 Rocky04 May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Seems like a typo, should be: // Test logically XOR

But I don't get why to mention the specific implementation instead of the goal. While a hint that this is a logical XOR is indeed nice I think a comment should primary explain the meaning and not the implementation detail. Like: // Logically XOR to only handle a state change

Copy link
Collaborator

@HiFiPhile HiFiPhile May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not a typo, it could be other consumers later, should be only called when whole value change to true or false.

This comment was marked as off-topic.

Sign in to view

if(!_usbd_dev.sof_consumer != !consumer_old) {
dcd_sof_enable(rhport, _usbd_dev.sof_consumer);
}
}

bool usbd_edpt_iso_alloc(uint8_t rhport, uint8_t ep_addr, uint16_t largest_packet_size) {
Expand Down
6 changes: 6 additions & 0 deletions src/device/usbd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ bool tud_disconnect(void);
// Return false on unsupported MCUs
bool tud_connect(void);

// Enable or disable the Start Of Frame callback support
bool tud_sof_cb_enable(bool en);

// Carry out Data and Status stage of control transfer
// - If len = 0, it is equivalent to sending status only
// - If len > wLength : it will be truncated
Expand Down Expand Up @@ -152,6 +155,9 @@ TU_ATTR_WEAK void tud_resume_cb(void);
// Invoked when there is a new usb event, which need to be processed by tud_task()/tud_task_ext()
void tud_event_hook_cb(uint8_t rhport, uint32_t eventid, bool in_isr);

// Invoked when a new (micro) frame started
void tud_sof_cb(uint32_t frame_count);

// Invoked when received control request with VENDOR TYPE
TU_ATTR_WEAK bool tud_vendor_control_xfer_cb(uint8_t rhport, uint8_t stage, tusb_control_request_t const * request);

Expand Down
11 changes: 10 additions & 1 deletion src/device/usbd_pvt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,15 @@

#define TU_LOG_USBD(...) TU_LOG(CFG_TUD_LOG_LEVEL, __VA_ARGS__)

//--------------------------------------------------------------------+
// MACRO CONSTANT TYPEDEF PROTYPES
//--------------------------------------------------------------------+

typedef enum {
SOF_CONSUMER_USER = 0,
SOF_CONSUMER_AUDIO,
} sof_consumer_t;

//--------------------------------------------------------------------+
// Class Driver API
//--------------------------------------------------------------------+
Expand Down Expand Up @@ -108,7 +117,7 @@ bool usbd_edpt_ready(uint8_t rhport, uint8_t ep_addr) {
}

// Enable SOF interrupt
void usbd_sof_enable(uint8_t rhport, bool en);
void usbd_sof_enable(uint8_t rhport, sof_consumer_t consumer, bool en);

/*------------------------------------------------------------------*/
/* Helper
Expand Down
Loading