Skip to content

Commit

Permalink
Add terminating resistor support
Browse files Browse the repository at this point in the history 
The mcba_usb kernel driver in linux has support for configurable terminating
resistors. This patch adds similar support to this firmware which the
gs_usb USB driver would then activate via the netlink interface.
  • Loading branch information
@Daniel-Trevitz
Daniel-Trevitz committed Sep 18, 2022
1 parent c19f3a1 commit 03de1b5
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 2 deletions.
5 changes: 5 additions & 0 deletions include/config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ THE SOFTWARE.
#define USB_Pin_DM GPIO_PIN_11
#define USB_Pin_DP GPIO_PIN_12

#define PIN_TERM_GPIO_Port GPIOB
#define PIN_TERM_Pin GPIO_PIN_3

This comment has been minimized.

Sign in to view

Copy link
@marckleinebudde

marckleinebudde Sep 19, 2022

Contributor

the indention of GPIO_PIN_3 looks strange

This comment has been minimized.

Sign in to view

Copy link
@Daniel-Trevitz

Daniel-Trevitz Sep 19, 2022

Author Contributor

The tabs in this file have been driving me nuts. I'll give it a third go.

This comment has been minimized.

Sign in to view

Copy link
@marckleinebudde

marckleinebudde Sep 19, 2022

Contributor

yeay - the coding style is a bit inconsistent
#define PIN_TERM_Mode GPIO_MODE_OUTPUT_PP
#define PIN_TERM_Active_High 1

#else
#error please define BOARD
#endif
3 changes: 3 additions & 0 deletions include/gpio.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,6 @@ THE SOFTWARE.
#pragma once

void gpio_init(void);

void set_term(unsigned int channel, int state);
int is_term_on(unsigned int channel);
3 changes: 3 additions & 0 deletions include/gs_usb.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ THE SOFTWARE.
* GS_USB_BREQ_BT_CONST_EXT and struct gs_device_bt_const_extended
*/
#define GS_CAN_FEATURE_BT_CONST_EXT (1<<10)
#define GS_CAN_FEATURE_TERMINATION (1<<11)

#define GS_CAN_FLAG_OVERFLOW (1<<0)
#define GS_CAN_FLAG_FD (1<<1) /* is a CAN-FD frame */
Expand Down Expand Up @@ -163,6 +164,8 @@ enum gs_usb_breq {
GS_USB_BREQ_SET_USER_ID,
GS_USB_BREQ_DATA_BITTIMING,
GS_USB_BREQ_BT_CONST_EXT,
GS_USB_BREQ_SET_TERMINATION,
GS_USB_BREQ_GET_TERMINATION
};

enum gs_can_mode {
Expand Down
47 changes: 47 additions & 0 deletions src/gpio.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,19 @@ void gpio_init(void)
GPIO_InitStruct.Speed = GPIO_SPEED_FREQ_LOW;
HAL_GPIO_Init(LEDTX_GPIO_Port, &GPIO_InitStruct);

#ifdef PIN_TERM_Pin
#if (PIN_TERM_Active_High == 1)
HAL_GPIO_WritePin(PIN_TERM_GPIO_Port, PIN_TERM_Pin, GPIO_PIN_RESET);
#else
HAL_GPIO_WritePin(PIN_TERM_GPIO_Port, PIN_TERM_Pin, GPIO_PIN_SET);
#endif
GPIO_InitStruct.Pin = PIN_TERM_Pin;
GPIO_InitStruct.Mode = PIN_TERM_Mode;
GPIO_InitStruct.Pull = GPIO_NOPULL;
GPIO_InitStruct.Speed = GPIO_SPEED_FREQ_LOW;
HAL_GPIO_Init(PIN_TERM_GPIO_Port, &GPIO_InitStruct);
#endif

#if defined(BOARD_cannette)
HAL_GPIO_WritePin(nCANSTBY_Port, nCANSTBY_Pin, GPIO_PIN_RESET);
GPIO_InitStruct.Pin = nCANSTBY_Pin;
Expand Down Expand Up @@ -104,3 +117,37 @@ void gpio_init(void)
HAL_GPIO_Init(USB_GPIO_Port, &GPIO_InitStruct);
#endif
}

static int term_state = 0;

void set_term(int channel, int state)
{
if (state)

This comment has been minimized.

Sign in to view

Copy link
@marckleinebudde

marckleinebudde Sep 19, 2022

Contributor 
if (state)
	term_state |= 1 << channel;
else
	term_state &= ~(1 << channel);

or

if (state) {
	term_state |= 1 << channel;
} else {
	term_state &= ~(1 << channel);
}

I think @fenugrec prefers the 2nd.

This comment has been minimized.

Sign in to view

Copy link
@Daniel-Trevitz

Daniel-Trevitz Sep 19, 2022

Author Contributor

I prefer the second of the two. Looking around it looks like candlelight uses the second.
{
term_state |= 1 << channel;
}
else
{
term_state &= ~(1 << channel);
}

// TODO add support for multiple channels
#ifndef PIN_TERM_Pin
(void)state;

This comment has been minimized.

Sign in to view

Copy link
@marckleinebudde

marckleinebudde Sep 19, 2022

Contributor

better return an error in this case

This comment has been minimized.

Sign in to view

Copy link
@Daniel-Trevitz

Daniel-Trevitz Sep 19, 2022

Author Contributor

You mean for the TODO?

This comment has been minimized.

Sign in to view

Copy link
@marckleinebudde

marckleinebudde Sep 19, 2022

Contributor

no - I mean in case no PIN_TERM_Pin is defined
#else
#if (PIN_TERM_Active_High == 1)
# define TERM_ON GPIO_PIN_SET
# define TERM_OFF GPIO_PIN_RESET
#else
# define TERM_ON GPIO_PIN_RESET
# define TERM_OFF GPIO_PIN_SET
#endif

HAL_GPIO_WritePin(PIN_TERM_GPIO_Port, PIN_TERM_Pin, state ? TERM_ON : TERM_OFF);
#endif
}

int is_term_on(unsigned int channel)
{
return !!(term_state & (1 << channel));
}
21 changes: 19 additions & 2 deletions src/usbd_gs_can.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ THE SOFTWARE.
#include "usbd_ioreq.h"
#include "gs_usb.h"
#include "can.h"
#include "gpio.h"
#include "timer.h"
#include "flash.h"

Expand Down Expand Up @@ -272,7 +273,11 @@ static const struct gs_device_bt_const USBD_GS_CAN_btconst = {
| GS_CAN_FEATURE_HW_TIMESTAMP
| GS_CAN_FEATURE_IDENTIFY
| GS_CAN_FEATURE_USER_ID
| GS_CAN_FEATURE_PAD_PKTS_TO_MAX_PKT_SIZE,
| GS_CAN_FEATURE_PAD_PKTS_TO_MAX_PKT_SIZE
#ifdef PIN_TERM_Pin
| GS_CAN_FEATURE_TERMINATION
#endif
,
CAN_CLOCK_SPEED, // can timing base clock
1, // tseg1 min
16, // tseg1 max
Expand Down Expand Up @@ -383,6 +388,11 @@ static uint8_t USBD_GS_CAN_EP0_RxReady(USBD_HandleTypeDef *pdev) {
}
break;

case GS_USB_BREQ_SET_TERMINATION:
memcpy(&param_u32, hcan->ep0_buf, sizeof(param_u32));
set_term(req->wValue, param_u32);

This comment has been minimized.

Sign in to view

Copy link
@marckleinebudde

marckleinebudde Sep 19, 2022

Contributor

please add error handling and issue a USBD_CtlError(pdev, req);

This comment has been minimized.

Sign in to view

Copy link
@Daniel-Trevitz

Daniel-Trevitz Sep 19, 2022

Author Contributor

Hmm, you mean for setting the terminating resistor when there is not one. The kernel driver should not be even making the call in that case; But I'll add the call all the same.

This comment has been minimized.

Sign in to view

Copy link
@marckleinebudde

marckleinebudde Sep 19, 2022

Contributor

Yes, the kernel should not, but do you trust the kernel?

When writing kernel code, I don't trust the USB devices either :)

This comment has been minimized.

Sign in to view

Copy link
@Daniel-Trevitz

Daniel-Trevitz Sep 19, 2022

Author Contributor

Fair enough; the way I looked at it was that it is a NOP, so rather than break everything because of a bad function call, just fail silently. That said, I think in this case failure won't cause the driver to delete itself like some other errors... so it should be okay.
break;

case GS_USB_BREQ_SET_USER_ID:
memcpy(&param_u32, hcan->ep0_buf, sizeof(param_u32));
if (flash_set_user_id(req->wValue, param_u32)) {
Expand Down Expand Up @@ -477,10 +487,17 @@ static uint8_t USBD_GS_CAN_Config_Request(USBD_HandleTypeDef *pdev, USBD_SetupRe
case GS_USB_BREQ_BITTIMING:
case GS_USB_BREQ_IDENTIFY:
case GS_USB_BREQ_SET_USER_ID:
case GS_USB_BREQ_SET_TERMINATION:
hcan->last_setup_request = *req;
USBD_CtlPrepareRx(pdev, hcan->ep0_buf, req->wLength);
break;

case GS_USB_BREQ_GET_TERMINATION:
d32 = is_term_on(req->wValue);
memcpy(hcan->ep0_buf, &d32, sizeof(d32));
USBD_CtlSendData(pdev, hcan->ep0_buf, req->wLength);
break;

This comment has been minimized.

Sign in to view

Copy link
@Daniel-Trevitz

Daniel-Trevitz Sep 19, 2022

Author Contributor

@marckleinebudde, What kind of error handling do we want here? I'm still fuzzy on the role of USBD_GS_CAN_Config_Request vs USBD_GS_CAN_EP0_RxReady

This comment has been minimized.

Sign in to view

Copy link
@marckleinebudde

marckleinebudde Sep 19, 2022

Contributor

I think USBD_GS_CAN_Config_Request() is called first and USBD_GS_CAN_EP0_RxReady() after the config request has been completely received.

So if the hardware doesn't support termination then in USBD_GS_CAN_Config_Request() issue a USBD_CtlError() otherwise send the answer with USBD_CtlSendData().

You can rename is_term_on() to int get_term(int channel, bool *enabled), return an error if the device doesn't support termination, if it does, assign enabled and return 0.
case GS_USB_BREQ_DEVICE_CONFIG:
memcpy(hcan->ep0_buf, &USBD_GS_CAN_dconf, sizeof(USBD_GS_CAN_dconf));
USBD_CtlSendData(pdev, hcan->ep0_buf, req->wLength);
Expand Down Expand Up @@ -738,7 +755,7 @@ void USBD_GS_CAN_SuspendCallback(USBD_HandleTypeDef *pdev)

if(hcan != NULL && hcan->leds != NULL)
led_set_mode(hcan->leds, led_mode_off);

is_usb_suspend_cb = true;
}

Expand Down

0 comments on commit 03de1b5

Please sign in to comment.