Add terminating resistor support #108

Daniel-Trevitz · 2022-09-15T21:10:29Z

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.

I have not tested this code on the device yet or with the linux driver. Currently, I want to know if the maintainers of candleLight are interested in this functionality.

Once I have tested the kernel module changes against these changes I plan to send a similar RFC for the kernel change.

fenugrec · 2022-09-15T21:27:39Z

(ref - see ticket #92 as well )

Daniel-Trevitz · 2022-09-16T10:57:04Z

@marckleinebudde, Have your efforts on this stalled?

marckleinebudde · 2022-09-18T13:28:35Z

Hello @Daniel-Trevitz,

yes, there was no effort other than the RFC. I'm not sure if @ryedwards build hardware with switchable termination yet. As mentioned in #92 this should be modeled the same way as the IDENTIFY feature. I'll create a patch for the Linux driver implementing these.

Daniel-Trevitz · 2022-09-18T19:16:55Z

gs_usb.c.txt

@marckleinebudde No need, done and works. Just finished testing.

marckleinebudde · 2022-09-18T20:25:59Z

Hello @Daniel-Trevitz,

I've made a pseudo PR to review your driver changes:
marckleinebudde/linux#9 (review)

Overloading the echo_id is hack and has no cahnce of going mainline. Here's my version of the driver patch:

https://lore.kernel.org/all/20220918202348.675850-1-mkl@pengutronix.de

Daniel-Trevitz · 2022-09-18T21:35:56Z

The revised pull request is not what I am testing with; I have a custom board that uses a neo pixel for status indication.
This is the subset that is relevant to candlelight, with an example added to the dev board for compile testing.

marckleinebudde · 2022-09-19T15:02:52Z

Let's add the interface documentation first, see #109.

Daniel-Trevitz · 2022-09-19T15:08:24Z

You want me to roll the difference from #109 into PR?

marckleinebudde · 2022-09-19T15:09:35Z

Yes, please rebase ontop of my PR.

Daniel-Trevitz · 2022-09-19T16:03:37Z

Force pushed an update. Still have to rebase off of #109 (learning how to do that...) and actually test it.

Daniel-Trevitz · 2022-09-19T16:54:34Z

Testing complete. Changed the dummy implementation in gpio to have fewer ifdefs. I tested by commenting / uncommenting the two new USBD_CtrlError while pretending that the feature was active.

I always get:
sudo ip link set dev can0 type can termination 120
RTNETLINK answers: Broken pipe

include/gpio.h

marckleinebudde · 2022-09-19T19:39:46Z

src/gpio.c

@@ -104,3 +118,50 @@ void gpio_init(void)
 	HAL_GPIO_Init(USB_GPIO_Port, &GPIO_InitStruct);
 #endif
 }
+
+#ifdef PIN_TERM_Pin
+static int term_state = 0;


no need to initialize static values to zero.

This was more or less a choice; specifically calling out zero because an alternative implementation could have a termination on by default.

That and I have a hard time trusting that the data is actually nulled... I mean, I know it is... I just don't trust it.

Uninitialized static variables end up in the BSS segment, which is guaranteed by C to be zeroed prior startup.

https://stackoverflow.com/questions/9535250/why-is-the-bss-segment-required

marckleinebudde · 2022-09-19T19:40:38Z

src/gpio.c

+
+#else
+
+enum terminator_status set_term(unsigned int channel, enum terminator_status state)


Can you place them as static inline functions into the header file?

Do you mean static inline the version that is a nop? I'm not inclined to expose term_state globally.

That does require we pull config.h into gpio.h. Do we care about header files doing that kind of stuff? I can't imagine we do, but might as well check...

marckleinebudde · 2022-09-19T19:41:19Z

src/usbd_gs_can.c

+		case GS_USB_BREQ_GET_TERMINATION:
+			term_state = get_term(req->wValue);
+
+			if(term_state == term_unsupported) {


marckleinebudde · 2022-09-19T19:43:44Z

src/usbd_gs_can.c

@@ -477,10 +489,24 @@ 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;


You can check here with get_term(req->wValue) here if that channel supports termination....

Yeah, that's fair. I had contemplated it, but was tending towards don't do extra work. That said, I think exiting the usb HAL stuff ASAP is better .... and that said I'm probably over thinking this ;)

marckleinebudde · 2022-09-19T19:45:53Z

src/gpio.c

@@ -70,6 +71,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 you move the termination gpio init into a separate function, you can have a static inline noop or a proper function.

ok; I contemplated the same but wanted to follow the existing style.

What'd be better is if we had some kind of isolated board support package solution. I know I'd be interested in upstreaming my neo pixel support, but it's just not possible with the way everything is done with ifdefs. I had to add DMA support, interrupts, activate a different GPIO clock, add a whole new c file, etc. etc.

marckleinebudde · 2022-09-19T19:50:22Z

Force pushed an update. Still have to rebase off of #109 (learning how to do that...) and actually test it.

Consider you're on your development branch.

# create a backup branch:
git branch backup

# fetch my PR branch into the special branch FETCH_HEAD
git fetch https://github.com/marckleinebudde/candleLight_fw.git termination

# rebase your development branch to FETCH_HEAD
git rebase FETCH_HEAD

# solve conflicts
git add -u
git rebase --continue

Daniel-Trevitz · 2022-09-19T21:14:42Z

rebase your development branch to FETCH_HEAD

git rebase FETCH_HEAD

Ah ha! I was missing this magic FETCH_HEAD. /thankyou/

Daniel-Trevitz · 2022-09-19T21:18:01Z

I've committed the new version with the rebase; I believe the static inline of get_term /set_term while the termination functionality is disabled addresses your underlying concern; not penalizing performance for something that is unused.

It does not fully inline it, since it would move the data to be a public symbol.

The candleLight community is working on switchable termination support for the candleLight firmware. As the the Linux CAN framework supports switchable termination add this feature to the gs_usb driver. Devices supporting the feature should set the GS_CAN_FEATURE_TERMINATION and implement the GS_USB_BREQ_SET_TERMINATION and GS_USB_BREQ_GET_TERMINATION control messages. For now the driver assumes for activated termination the standard termination value of 120Ω. Link: https://lore.kernel.org/all/20220923074114.662045-1-mkl@pengutronix.de Link: candle-usb/candleLight_fw#92 Link: candle-usb/candleLight_fw#109 Link: candle-usb/candleLight_fw#108 Cc: Daniel Trevitz <daniel.trevitz@wika.com> Cc: Ryan Edwards <ryan.edwards@gmail.com> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

fenugrec

Thanks for working on this.

fenugrec · 2022-10-05T01:13:05Z

src/gpio.c

+
+enum terminator_status get_term(unsigned int channel)
+{
+	return !!(term_state & (1 << channel));


Not sure how I feel about using !! to cast to bool and then implicit casting to enum - I see what you're trying to do, but it requires one to go back to gpio.h to make sure the enum members are in the right order with the right values. And everything breaks if term_inactive is not 0 (for example if the currently hardcoded value of term_unsupported is changed, or another item added before term_inactive.

I'd like to believe that a simple if state & ( 1<< chan)) { return term_active } else { return term_inactive} would probably compile to the exact same machine code while being more robust and readable ?

I doubt the compiler will remove the conditional, but it'll be interesting to check. I'll make the change at some point here.

I doubt the compiler will remove the conditional

Just checked on godbolt.org and it compiles to same code at -O1 and higher (we use -Os) :

enum terminator_status get_term(unsigned int channel) { return !!(term_state & (1 << channel)); } enum terminator_status get_term2(unsigned int channel) { if (term_state & (1 << channel)) { return term_active; } else { return term_inactive; } } ******************************************** get_term: ldr r3, .L2 ldr r3, [r3] asrs r3, r3, r0 movs r0, r3 movs r3, #1 ands r0, r3 bx lr .L2: .word .LANCHOR0 get_term2: ldr r3, .L5 ldr r3, [r3] asrs r3, r3, r0 movs r0, r3 movs r3, #1 ands r0, r3 bx lr .L5:

Therefore I would prefer if you changed it. Thanks !

(Interestingly, c++ doesn't let you cast bool to enum).

awesome. thanks for checking.

fenugrec · 2022-10-05T01:17:18Z

src/gpio.c

-#else
-	HAL_GPIO_WritePin(LEDRX_GPIO_Port, LEDRX_Pin, GPIO_PIN_SET);
-#endif
+	HAL_GPIO_WritePin(LEDRX_GPIO_Port, LEDRX_Pin, GPIO_INIT_STATE(LEDRX_Active_High));


Ok but this belongs in a separate commit (that would add your GPIO_INIT_STATE macro), as it is not related to terminations

Do you want this change? Do you want it in a separate pull request?

Yes please - one small commit for your GPIO_INIT_STATE changes, then the rest of your changes. I just merged #109 so you would need to rebase anyway I think

marckleinebudde mentioned this pull request Sep 18, 2022

HACK: gs_usb: add termination support marckleinebudde/linux#9

Closed

Daniel-Trevitz force-pushed the master branch 2 times, most recently from 59b069b to 923ba2a Compare September 18, 2022 21:33

Daniel-Trevitz marked this pull request as ready for review September 18, 2022 21:33

Daniel-Trevitz force-pushed the master branch from 923ba2a to 03de1b5 Compare September 18, 2022 21:36

Daniel-Trevitz changed the title ~~[RFC] Add terminating resistor support~~ Add terminating resistor support Sep 18, 2022

marckleinebudde mentioned this pull request Sep 19, 2022

gs_usb: document switchable termination support #109

Merged

Daniel-Trevitz force-pushed the master branch 2 times, most recently from b7f5ea5 to 81dc061 Compare September 19, 2022 16:02

Daniel-Trevitz force-pushed the master branch 2 times, most recently from f5aa52a to e6b51fa Compare September 19, 2022 16:45

marckleinebudde suggested changes Sep 19, 2022

View reviewed changes

Daniel-Trevitz force-pushed the master branch from e6b51fa to 8919e6f Compare September 19, 2022 21:14

fenugrec requested changes Oct 5, 2022

View reviewed changes

Daniel-Trevitz closed this Oct 5, 2022

Daniel-Trevitz force-pushed the master branch from 8919e6f to 151ec30 Compare October 5, 2022 22:10


		#else

		enum terminator_status set_term(unsigned int channel, enum terminator_status state)

Add terminating resistor support #108

Add terminating resistor support #108

Conversation

Daniel-Trevitz commented Sep 15, 2022

fenugrec commented Sep 15, 2022

Daniel-Trevitz commented Sep 16, 2022

marckleinebudde commented Sep 18, 2022

Daniel-Trevitz commented Sep 18, 2022

marckleinebudde commented Sep 18, 2022 • edited

Daniel-Trevitz commented Sep 18, 2022

marckleinebudde commented Sep 19, 2022

Daniel-Trevitz commented Sep 19, 2022 • edited

marckleinebudde commented Sep 19, 2022

Daniel-Trevitz commented Sep 19, 2022

Daniel-Trevitz commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marckleinebudde Sep 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marckleinebudde commented Sep 19, 2022

Daniel-Trevitz commented Sep 19, 2022 • edited

rebase your development branch to FETCH_HEAD

Daniel-Trevitz commented Sep 19, 2022

fenugrec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marckleinebudde commented Sep 18, 2022 •

edited

Daniel-Trevitz commented Sep 19, 2022 •

edited

marckleinebudde Sep 20, 2022 •

edited

Daniel-Trevitz commented Sep 19, 2022 •

edited