From 230e87b0acaa63f486161a13e1eb33f1040909f0 Mon Sep 17 00:00:00 2001 From: Henrik Brix Andersen Date: Fri, 25 Oct 2024 08:27:38 +0000 Subject: [PATCH] usb: device: class: gs_usb: unify application event callbacks Unify the application event callbacks to reduce code complexity and simplify future event additions. Signed-off-by: Henrik Brix Andersen --- app/src/cannectivity.h | 29 +------ app/src/led.c | 96 +++++++++------------- app/src/main.c | 4 +- include/cannectivity/usb/class/gs_usb.h | 60 ++++++-------- subsys/usb/device/class/gs_usb.c | 31 +++---- subsys/usb/device_next/class/gs_usb.c | 31 +++---- tests/subsys/usb/gs_usb/host/src/main.c | 43 +++++----- tests/subsys/usb/gs_usb/host/testcase.yaml | 4 +- 8 files changed, 123 insertions(+), 175 deletions(-) diff --git a/app/src/cannectivity.h b/app/src/cannectivity.h index d557bde..d9fbc69 100644 --- a/app/src/cannectivity.h +++ b/app/src/cannectivity.h @@ -35,37 +35,16 @@ DT_FOREACH_CHILD_STATUS_OKAY_SEP(CANNECTIVITY_DT_NODE_ID, fn, sep) /** - * @brief CANnectivity CAN channel LED identify callback + * @brief CANnectivity CAN channel LED event callback * * @param dev Pointer to the device structure for the driver instance. * @param ch CAN channel number. - * @param identify True if the channel identify is active, false otherwise. + * @param event Channel event. * @param user_data User data provided when registering the callback. * @return 0 on success, negative error number otherwise. */ -int cannectivity_led_identify(const struct device *dev, uint16_t ch, bool identify, - void *user_data); - -/** - * @brief CANnectivity CAN channel LED state callback - * - * @param dev Pointer to the device structure for the driver instance. - * @param ch CAN channel number. - * @param started True if the channel is started, false otherwise. - * @param user_data User data provided when registering the callback. - * @return 0 on success, negative error number otherwise. - */ -int cannectivity_led_state(const struct device *dev, uint16_t ch, bool started, void *user_data); - -/** - * @brief CANnectivity CAN channel LED activity callback - * - * @param dev Pointer to the device structure for the driver instance. - * @param ch CAN channel number. - * @param user_data User data provided when registering the callback. - * @return 0 on success, negative error number otherwise. - */ -int cannectivity_led_activity(const struct device *dev, uint16_t ch, void *user_data); +int cannectivity_led_event(const struct device *dev, uint16_t ch, enum gs_usb_event event, + void *user_data); /** * @brief CANnectivity LED initialization function diff --git a/app/src/led.c b/app/src/led.c index 25e36e3..5592990 100644 --- a/app/src/led.c +++ b/app/src/led.c @@ -335,58 +335,10 @@ static void led_tick(struct k_timer *timer) } } -int cannectivity_led_identify(const struct device *dev, uint16_t ch, bool identify, void *user_data) -{ - led_event_t event; - int err; - - ARG_UNUSED(dev); - ARG_UNUSED(user_data); - - LOG_DBG("identify channel %u %s", ch, identify ? "on" : "off"); - - if (identify) { - event = LED_EVENT_CHANNEL_IDENTIFY_ON; - } else { - event = LED_EVENT_CHANNEL_IDENTIFY_OFF; - } - - err = led_event_enqueue(ch, event); - if (err != 0) { - LOG_ERR("failed to enqueue identify %s event for channel %u (err %d)", - event == LED_EVENT_CHANNEL_IDENTIFY_ON ? "on" : "off", ch, err); - } - - return 0; -} - -int cannectivity_led_state(const struct device *dev, uint16_t ch, bool started, void *user_data) -{ - led_event_t event; - int err; - - ARG_UNUSED(dev); - ARG_UNUSED(user_data); - - LOG_DBG("channel %u %s", ch, started ? "started" : "stopped"); - - if (started) { - event = LED_EVENT_CHANNEL_STARTED; - } else { - event = LED_EVENT_CHANNEL_STOPPED; - } - - err = led_event_enqueue(ch, event); - if (err != 0) { - LOG_ERR("failed to enqueue channel %s event for channel %u (err %d)", - event == LED_EVENT_CHANNEL_STOPPED ? "stopped" : "started", ch, err); - } - - return 0; -} - -int cannectivity_led_activity(const struct device *dev, uint16_t ch, void *user_data) +int cannectivity_led_event(const struct device *dev, uint16_t ch, enum gs_usb_event event, + void *user_data) { + led_event_t led_event; struct led_ctx *lctx; int err; @@ -394,23 +346,49 @@ int cannectivity_led_activity(const struct device *dev, uint16_t ch, void *user_ ARG_UNUSED(user_data); if (ch >= ARRAY_SIZE(led_channel_ctx)) { - LOG_ERR("activity event for non-existing channel %u", ch); + LOG_ERR("event for non-existing channel %u", ch); return -EINVAL; } lctx = &led_channel_ctx[ch]; - /* low-pass filter activity events */ - if (sys_timepoint_expired(lctx->activity)) { + switch (event) { + case GS_USB_EVENT_CHANNEL_STARTED: + LOG_DBG("channel %u started", ch); + led_event = LED_EVENT_CHANNEL_STARTED; + break; + case GS_USB_EVENT_CHANNEL_STOPPED: + LOG_DBG("channel %u stopped", ch); + led_event = LED_EVENT_CHANNEL_STOPPED; + break; + case GS_USB_EVENT_CHANNEL_ACTIVITY: + /* low-pass filter activity events */ + if (!sys_timepoint_expired(lctx->activity)) { + goto skipped; + } + lctx->activity = sys_timepoint_calc(K_MSEC(LED_TICK_MS * LED_TICKS_ACTIVITY)); + led_event = LED_EVENT_CHANNEL_ACTIVITY; + break; + case GS_USB_EVENT_CHANNEL_IDENTIFY_ON: + LOG_DBG("identify channel %u on", ch); + led_event = LED_EVENT_CHANNEL_IDENTIFY_ON; + break; + case GS_USB_EVENT_CHANNEL_IDENTIFY_OFF: + LOG_DBG("identify channel %u off", ch); + led_event = LED_EVENT_CHANNEL_IDENTIFY_OFF; + break; + default: + /* Unsupported event */ + goto skipped; + } - err = led_event_enqueue(ch, LED_EVENT_CHANNEL_ACTIVITY); - if (err != 0) { - LOG_ERR("failed to enqueue channel activity event for channel %u (err %d)", - ch, err); - } + err = led_event_enqueue(ch, led_event); + if (err != 0) { + LOG_ERR("failed to enqueue LED event for channel %u (err %d)", ch, err); } +skipped: return 0; } diff --git a/app/src/main.c b/app/src/main.c index b1e78dc..3556e5b 100644 --- a/app/src/main.c +++ b/app/src/main.c @@ -23,9 +23,7 @@ static const struct gs_usb_ops gs_usb_ops = { .timestamp = cannectivity_timestamp_get, #endif #ifdef CONFIG_CANNECTIVITY_LED_GPIO - .identify = cannectivity_led_identify, - .state = cannectivity_led_state, - .activity = cannectivity_led_activity, + .event = cannectivity_led_event, #endif #ifdef CONFIG_CANNECTIVITY_TERMINATION_GPIO .set_termination = cannectivity_set_termination, diff --git a/include/cannectivity/usb/class/gs_usb.h b/include/cannectivity/usb/class/gs_usb.h index f89b5e6..dff440e 100644 --- a/include/cannectivity/usb/class/gs_usb.h +++ b/include/cannectivity/usb/class/gs_usb.h @@ -507,6 +507,26 @@ struct gs_usb_host_frame_hdr { /** @} */ +/** + * @brief Channel events. + * + * @see gs_usb_event_callback_t + */ +enum gs_usb_event { + /** Channel started. */ + GS_USB_EVENT_CHANNEL_STARTED, + /** Channel stopped. */ + GS_USB_EVENT_CHANNEL_STOPPED, + /** Channel RX/TX activity. */ + GS_USB_EVENT_CHANNEL_ACTIVITY, +#if defined(CONFIG_USB_DEVICE_GS_USB_IDENTIFICATION) || defined(CONFIG_USBD_GS_USB_IDENTIFICATION) + /** Visual channel identification on. */ + GS_USB_EVENT_CHANNEL_IDENTIFY_ON, + /** Visual channel identification off. */ + GS_USB_EVENT_CHANNEL_IDENTIFY_OFF, +#endif +}; + /** * @brief Custom (random) MSOSv2 vendor code */ @@ -539,18 +559,6 @@ void gs_usb_register_vendorcode_callback(gs_usb_vendorcode_callback_t callback); typedef int (*gs_usb_timestamp_callback_t)(const struct device *dev, uint32_t *timestamp, void *user_data); -/** - * @brief Defines the callback signature for visually identifying a given CAN channel - * - * @param dev Pointer to the device structure for the driver instance. - * @param ch CAN channel number. - * @param identify True if the channel identify is active, false otherwise. - * @param user_data User data provided when registering the callback. - * @return 0 on success, negative error number otherwise. - */ -typedef int (*gs_usb_identify_callback_t)(const struct device *dev, uint16_t ch, bool identify, - void *user_data); - /** * @brief Defines the callback signature for setting the bus termination of a given CAN channel * @@ -576,26 +584,16 @@ typedef int (*gs_usb_get_termination_callback_t)(const struct device *dev, uint1 bool *terminated, void *user_data); /** - * @brief Defines the callback signature for reporting the state of a given CAN channel - * - * @param dev Pointer to the device structure for the driver instance. - * @param ch CAN channel number. - * @param started True if the channel is started, false otherwise. - * @param user_data User data provided when registering the callback. - * @return 0 on success, negative error number otherwise. - */ -typedef int (*gs_usb_state_callback_t)(const struct device *dev, uint16_t ch, bool started, - void *user_data); - -/** - * @brief Defines the callback signature for reporting RX/TX activity of a given CAN channel + * @brief Defines the callback signature for reporting events for a given CAN channel * * @param dev Pointer to the device structure for the driver instance. * @param ch CAN channel number. + * @param event Event type. * @param user_data User data provided when registering the callback. * @return 0 on success, negative error number otherwise. */ -typedef int (*gs_usb_activity_callback_t)(const struct device *dev, uint16_t ch, void *user_data); +typedef int (*gs_usb_event_callback_t)(const struct device *dev, uint16_t ch, + enum gs_usb_event event, void *user_data); /** * @brief Callback operations structure. @@ -611,14 +609,8 @@ struct gs_usb_ops { /** Optional CAN channel get termination callback */ gs_usb_get_termination_callback_t get_termination; #endif -#if defined(CONFIG_USB_DEVICE_GS_USB_IDENTIFICATION) || defined(CONFIG_USBD_GS_USB_IDENTIFICATION) - /** Optional CAN channel identify callback */ - gs_usb_identify_callback_t identify; -#endif - /** CAN channel state callback */ - gs_usb_state_callback_t state; - /** CAN channel activity callback */ - gs_usb_activity_callback_t activity; + /** CAN channel event callback */ + gs_usb_event_callback_t event; }; /** diff --git a/subsys/usb/device/class/gs_usb.c b/subsys/usb/device/class/gs_usb.c index 5916f0c..bf96307 100644 --- a/subsys/usb/device/class/gs_usb.c +++ b/subsys/usb/device/class/gs_usb.c @@ -493,6 +493,7 @@ static int gs_usb_request_mode(const struct device *dev, uint16_t ch, int32_t tl struct gs_usb_data *data = dev->data; struct gs_usb_channel_data *channel; can_mode_t mode = CAN_MODE_NORMAL; + enum gs_usb_event event; uint32_t flags; int err; @@ -569,8 +570,10 @@ static int gs_usb_request_mode(const struct device *dev, uint16_t ch, int32_t tl return -ENOTSUP; } - if (data->ops.state != NULL) { - err = data->ops.state(dev, channel->ch, channel->started, data->user_data); + if (data->ops.event != NULL) { + event = channel->started ? GS_USB_EVENT_CHANNEL_STARTED : + GS_USB_EVENT_CHANNEL_STOPPED; + err = data->ops.event(dev, channel->ch, event, data->user_data); if (err != 0) { LOG_ERR("failed to report channel %u state change (err %d)", channel->ch, err); @@ -583,15 +586,11 @@ static int gs_usb_request_mode(const struct device *dev, uint16_t ch, int32_t tl static int gs_usb_request_identify(const struct device *dev, uint16_t ch, int32_t tlen, uint8_t *tdata) { +#ifdef CONFIG_USB_DEVICE_GS_USB_IDENTIFICATION struct gs_usb_identify_mode *im = (struct gs_usb_identify_mode *)tdata; struct gs_usb_data *data = dev->data; + enum gs_usb_event event; uint32_t mode; - bool identify; - - if (data->ops.identify == NULL) { - LOG_ERR("identify not supported"); - return -ENOTSUP; - } if (ch >= data->nchannels) { LOG_ERR("identify request for non-existing channel %u", ch); @@ -607,17 +606,20 @@ static int gs_usb_request_identify(const struct device *dev, uint16_t ch, int32_ switch (mode) { case GS_USB_CHANNEL_IDENTIFY_MODE_OFF: - identify = false; + event = GS_USB_EVENT_CHANNEL_IDENTIFY_OFF; break; case GS_USB_CHANNEL_IDENTIFY_MODE_ON: - identify = true; + event = GS_USB_EVENT_CHANNEL_IDENTIFY_ON; break; default: LOG_ERR("unsupported identify mode %d for channel %u", mode, ch); return -ENOTSUP; } - return data->ops.identify(dev, ch, identify, data->user_data); + return data->ops.event(dev, ch, event, data->user_data); +#else /* CONFIG_USB_DEVICE_GS_USB_IDENTIFICATION */ + return -ENOTSUP; +#endif /* !CONFIG_USB_DEVICE_GS_USB_IDENTIFICATION */ } static int gs_usb_request_device_config(const struct device *dev, int32_t *tlen, uint8_t **tdata) @@ -975,8 +977,9 @@ static void gs_usb_rx_thread(void *p1, void *p2, void *p3) continue; } - if (data->ops.activity != NULL) { - err = data->ops.activity(dev, ch, data->user_data); + if (data->ops.event != NULL) { + err = data->ops.event(dev, ch, GS_USB_EVENT_CHANNEL_ACTIVITY, + data->user_data); if (err != 0) { LOG_ERR("activity callback for channel %u failed (err %d)", ch, err); @@ -1163,7 +1166,7 @@ static uint32_t gs_usb_features_from_ops(struct gs_usb_ops *ops) features |= GS_USB_CAN_FEATURE_HW_TIMESTAMP; } - if (ops->identify != NULL) { + if (UTIL_AND(IS_ENABLED(CONFIG_USB_DEVICE_GS_USB_IDENTIFICATION), ops->event != NULL)) { features |= GS_USB_CAN_FEATURE_IDENTIFY; } diff --git a/subsys/usb/device_next/class/gs_usb.c b/subsys/usb/device_next/class/gs_usb.c index bdcee0a..6dd33b7 100644 --- a/subsys/usb/device_next/class/gs_usb.c +++ b/subsys/usb/device_next/class/gs_usb.c @@ -496,6 +496,7 @@ static int gs_usb_request_mode(const struct device *dev, uint16_t ch, struct gs_usb_channel_data *channel; struct gs_usb_device_mode *dm = (struct gs_usb_device_mode *)buf->data; can_mode_t mode = CAN_MODE_NORMAL; + enum gs_usb_event event; uint32_t flags; int err; @@ -572,8 +573,10 @@ static int gs_usb_request_mode(const struct device *dev, uint16_t ch, return -ENOTSUP; } - if (data->ops.state != NULL) { - err = data->ops.state(dev, channel->ch, channel->started, data->user_data); + if (data->ops.event != NULL) { + event = channel->started ? GS_USB_EVENT_CHANNEL_STARTED : + GS_USB_EVENT_CHANNEL_STOPPED; + err = data->ops.event(dev, channel->ch, event, data->user_data); if (err != 0) { LOG_ERR("failed to report channel %u state change (err %d)", channel->ch, err); @@ -586,15 +589,11 @@ static int gs_usb_request_mode(const struct device *dev, uint16_t ch, static int gs_usb_request_identify(const struct device *dev, uint16_t ch, const struct net_buf *const buf) { +#ifdef CONFIG_USBD_GS_USB_IDENTIFICATION struct gs_usb_data *data = dev->data; struct gs_usb_identify_mode *im = (struct gs_usb_identify_mode *)buf->data; + enum gs_usb_event event; uint32_t mode; - bool identify; - - if (data->ops.identify == NULL) { - LOG_ERR("identify not supported"); - return -ENOTSUP; - } if (ch >= data->nchannels) { LOG_ERR("identify request for non-existing channel %u", ch); @@ -610,17 +609,20 @@ static int gs_usb_request_identify(const struct device *dev, uint16_t ch, switch (mode) { case GS_USB_CHANNEL_IDENTIFY_MODE_OFF: - identify = false; + event = GS_USB_EVENT_CHANNEL_IDENTIFY_OFF; break; case GS_USB_CHANNEL_IDENTIFY_MODE_ON: - identify = true; + event = GS_USB_EVENT_CHANNEL_IDENTIFY_ON; break; default: LOG_ERR("unsupported identify mode %d for channel %u", mode, ch); return -ENOTSUP; } - return data->ops.identify(dev, ch, identify, data->user_data); + return data->ops.event(dev, ch, event, data->user_data); +#else /* CONFIG_USBD_GS_USB_IDENTIFICATION */ + return -ENOTSUP; +#endif /* !CONFIG_USBD_GS_USB_IDENTIFICATION */ } static int gs_usb_request_device_config(const struct device *dev, struct net_buf *const buf) @@ -1086,8 +1088,9 @@ static void gs_usb_rx_thread(void *p1, void *p2, void *p3) continue; } - if (data->ops.activity != NULL) { - err = data->ops.activity(dev, ch, data->user_data); + if (data->ops.event != NULL) { + err = data->ops.event(dev, ch, GS_USB_EVENT_CHANNEL_ACTIVITY, + data->user_data); if (err != 0) { LOG_ERR("activity callback for channel %u failed (err %d)", ch, err); @@ -1338,7 +1341,7 @@ static uint32_t gs_usb_features_from_ops(struct gs_usb_ops *ops) features |= GS_USB_CAN_FEATURE_HW_TIMESTAMP; } - if (ops->identify != NULL) { + if (UTIL_AND(IS_ENABLED(CONFIG_USBD_GS_USB_IDENTIFICATION), ops->event != NULL)) { features |= GS_USB_CAN_FEATURE_IDENTIFY; } diff --git a/tests/subsys/usb/gs_usb/host/src/main.c b/tests/subsys/usb/gs_usb/host/src/main.c index 5027769..e76a7c0 100644 --- a/tests/subsys/usb/gs_usb/host/src/main.c +++ b/tests/subsys/usb/gs_usb/host/src/main.c @@ -81,30 +81,27 @@ static int fake_can_set_mode_delegate(const struct device *dev, can_mode_t mode) return 0; } -static int identify_cb(const struct device *dev, uint16_t ch, bool identify, void *user_data) +static int event_cb(const struct device *dev, uint16_t ch, enum gs_usb_event event, void *user_data) { uint32_t ud = POINTER_TO_UINT(user_data); - LOG_DBG("dev = %s, ch = %u, identify = %u, user_data = 0x%08x", dev->name, ch, identify, - ud); - - return 0; -} - -static int state_cb(const struct device *dev, uint16_t ch, bool started, void *user_data) -{ - uint32_t ud = POINTER_TO_UINT(user_data); - - LOG_DBG("dev = %s, ch = %u, started = %u, user_data = 0x%08x", dev->name, ch, started, ud); - - return 0; -} - -static int activity_cb(const struct device *dev, uint16_t ch, void *user_data) -{ - uint32_t ud = POINTER_TO_UINT(user_data); - - LOG_DBG("dev = %s, ch = %u, user_data = 0x%08x", dev->name, ch, ud); + switch (event) { + case GS_USB_EVENT_CHANNEL_STARTED: + LOG_DBG("dev = %s, ch = %u, started = 1, user_data = 0x%08x", dev->name, ch, ud); + break; + case GS_USB_EVENT_CHANNEL_STOPPED: + LOG_DBG("dev = %s, ch = %u, started = 0, user_data = 0x%08x", dev->name, ch, ud); + break; + case GS_USB_EVENT_CHANNEL_ACTIVITY: + LOG_DBG("dev = %s, ch = %u, activity = 1, user_data = 0x%08x", dev->name, ch, ud); + break; + case GS_USB_EVENT_CHANNEL_IDENTIFY_ON: + LOG_DBG("dev = %s, ch = %u, identify = 1, user_data = 0x%08x", dev->name, ch, ud); + break; + case GS_USB_EVENT_CHANNEL_IDENTIFY_OFF: + LOG_DBG("dev = %s, ch = %u, identify = 0, user_data = 0x%08x", dev->name, ch, ud); + break; + } return 0; } @@ -144,9 +141,7 @@ static int timestamp_get_cb(const struct device *dev, uint32_t *timestamp, void static const struct gs_usb_ops gs_usb_ops = { .timestamp = timestamp_get_cb, - .identify = identify_cb, - .state = state_cb, - .activity = activity_cb, + .event = event_cb, .set_termination = set_termination_cb, .get_termination = get_termination_cb, }; diff --git a/tests/subsys/usb/gs_usb/host/testcase.yaml b/tests/subsys/usb/gs_usb/host/testcase.yaml index 5852652..61ba179 100644 --- a/tests/subsys/usb/gs_usb/host/testcase.yaml +++ b/tests/subsys/usb/gs_usb/host/testcase.yaml @@ -13,10 +13,10 @@ tests: usb.gs_usb.host: depends_on: usb_device can integration_platforms: - - frdm_k64f + - frdm_k64f/mk64f12 usb.gs_usb.host.usbd_next: depends_on: usbd can integration_platforms: - - frdm_k64f + - frdm_k64f/mk64f12 extra_args: - FILE_SUFFIX=usbd_next