hathach / tinyusb

An open source cross-platform USB stack for embedded system
https://www.tinyusb.org
MIT License
5k stars 1.05k forks source link

add dcd_disconnect() dcd_connect() #327

Closed hathach closed 4 years ago

hathach commented 4 years ago

these should be soft disconect/connect via registers. Some MCUs may not support this features.

pigrew commented 4 years ago

@hathach Could you better describe what these functions should do?

Do they enable/disable the pull-up resistors on D+/D-? Is there anything else they would do?

On some STM32, there is no internal pull-up, so for them, there would need to be BSP support (perhaps implemented by using weak functions?).

hathach commented 4 years ago

@hathach Could you better describe what these functions should do?

Do they enable/disable the pull-up resistors on D+/D-? Is there anything else they would do?

@pigrew yeah, these disables the internal pullup resistor on D+/D-. That's it. It is enough to cause the disconnection on both host & device side.

On some STM32, there is no internal pull-up, so for them, there would need to be BSP support (perhaps implemented by using weak functions?).

Yeah, several old MCUs doesn't have internal pull-up and replied on the external GPIO. For these case, I think it is better to have dcd_disconnect()/dcd_connect() as weak function. The public version tud_disconnect/tud_connect will have warning attribute to let user know that this functionality is not supported on this mcu.

We don't need to provide bsp hook in this case, since the stack doesn't use these at all. It is provided for user convenience. Application should know their own way (GPIO) to disconnect/connect when working on those mcu.

pigrew commented 4 years ago

Should the dcd_init() function enable the PU, if it exists?

Should the weak dcd_connect() give a TU_LOG1 if it is called on a board that doesn't support it? Or do we just not define the function on these, forcing the BSP to implement it?

(STM32 F1/F2/F3 discovery boards do not have software control of the PU.)

The public version tud_disconnect/tud_connect will have warning attribute to let user know that this functionality is not supported on this mcu.

How would the tud_connect know if the functionality is supported by the MCU? I don't see any method.

hathach commented 4 years ago

Should the dcd_init() function enable the PU, if it exists?

Yes, dcd_init() should enable PU if existed

Should the weak dcd_connect() give a TU_LOG1 if it is called on a board that doesn't support it? Or do we just not define the function on these, forcing the BSP to implement it?

(STM32 F1/F2/F3 discovery boards do not have software control of the PU.) How would the tud_connect know if the functionality is supported by the MCU? I don't see any method.

I am wrong about function attribute, apparently we only put warning on the function prototype not implementation. However, since it It is a weak function,. we can have tud_disconnect() return false for mcu that not support that.

I made an PR to implement this for samd and nrf here. And yes, we add an LOG to TU_VERIFY of tud_connect/disconnect as well. Though most people won't probably use LOG, it may not very obvious. https://github.com/hathach/tinyusb/pull/330

hathach commented 4 years ago

I just realized, if the dcd_disconnect is left unimplemented by stm32f1, bsp can actually implemented it with their own external gpio version. They would be perfect, though we may need to document it somewhere :D. The stack currently makes no use of this pair of functions, so it is totally optional

pigrew commented 4 years ago

@hathach ,

I just realized, if the dcd_disconnect is left unimplemented by stm32f1, bsp can actually implemented it with their own external gpio version. They would be perfect, though we may need to document it somewhere :D. The stack currently makes no use of this pair of functions, so it is totally optional

Yes, I think that this is the way to go. If dcd_init leaves the pull-ups disabled, this even allows the BSP to override them in the case there are both internal and external pull-ups.

Could you look over my patch #332?

Oh, that's almost exactly what you did in #330. I'll review that patch, and see what I think, too.

Thanks!

hathach commented 4 years ago

@hathach ,

I just realized, if the dcd_disconnect is left unimplemented by stm32f1, bsp can actually implemented it with their own external gpio version. They would be perfect, though we may need to document it somewhere :D. The stack currently makes no use of this pair of functions, so it is totally optional

Yes, I think that this is the way to go. If dcd_init leaves the pull-ups disabled, this even allows the BSP to override them in the case there are both internal and external pull-ups.

I don't think people will use external PU when internal is available. Also TinyUSB shouldn't concern with external PU at all, it is up to BSP. Though I agree dcd_init() leave the PU disabled is fine, however, it doesn't really need to explicitly disable PU if it is enabled by default on power.

Could you look over my patch #332?

Oh, that's almost exactly what you did in #330. I'll review that patch, and see what I think, too.

Thanks!

Sure I will do

hathach commented 4 years ago

all port has implemented this API phew, thank you everyone for implementation and review.