-
Notifications
You must be signed in to change notification settings - Fork 7.8k
boards: pjrc: Added Support for the Teensy CLI Tools soft reboot functionality #88024
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.
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
base: main
Are you sure you want to change the base?
Conversation
@@ -13,6 +13,33 @@ config BUILD_OUTPUT_HEX | |||
config DISK_DRIVER_SDMMC | |||
default y if DISK_DRIVERS | |||
|
|||
config BOOTLOADER_TEENSY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a defconfig option, move to Kconfig
select SERIAL | ||
select USB_DEVICE_STACK | ||
select USB_CDC_ACM | ||
select CDC_ACM_DTE_RATE_CALLBACK_SUPPORT | ||
select USB_DEVICE_INITIALIZE_AT_BOOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfischer-no for review
config BOOTLOADER_TEENSY_DEVICE_NAME | ||
string "Teensy CDC ACM device name" | ||
depends on BOOTLOADER_TEENSY | ||
default "USB CDC-ACM" | ||
help | ||
Sets the CDC ACM port to watch for reboot commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would also move
|
||
static void teensy_reset(const struct device *dev, uint32_t rate) | ||
{ | ||
if (rate != 134) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is 134?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the baudrate the teensy cli tool sets when requesting a soft reboot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use defines/consts for these with descriptive names
/* The programmer set the baud rate to 134 baud. Reset into the | ||
* bootloader. | ||
*/ | ||
__ASM volatile("bkpt #251"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and 251?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bkpt #251
is the assembly command to enter the bootloader.
return cdc_acm_dte_rate_callback_set(dev, teensy_reset); | ||
} | ||
|
||
SYS_INIT(teensy_init, APPLICATION, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use board hook
The |
if BOOTLOADER_TEENSY | ||
|
||
config USB_DEVICE_VID | ||
default 0x16c0 | ||
|
||
config USB_DEVICE_PID | ||
default 0x0483 | ||
|
||
endif # BOOTLOADER_TEENSY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea why and whose VID this is, but it should not be here.
bool "Teensy bootloader support" | ||
select SERIAL | ||
select USB_DEVICE_STACK | ||
select USB_CDC_ACM | ||
select CDC_ACM_DTE_RATE_CALLBACK_SUPPORT | ||
select USB_DEVICE_INITIALIZE_AT_BOOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think a board can have its own USB application at board level. Also IMO what the Bossa support at SAM0 SoC level is just wrong. There should be some common place for the bootloader utilities, if at all.
https://docs.zephyrproject.org/latest/hardware/porting/board_porting.html#general-recommendations
@jbehrensnx PTAL the comments and CI issues |
I already implemented some of the feedback (through not pushed yet), but I am not sure when I will have time to work on this in the next couple of weeks. |
Moved the bootloader to the soc section to better accommodate other boards, hopefully implemented all feedback and created a snippet for the bootloader, offloading some of the Kconfig configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove from IMXRT SOC folder, this is board specific and will not be maintained by NXP maintainers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that, but the intention of moving from boards
to the soc
folder was to facilitate support of boards that are either teensy clones or based on the teensy boards and thus are using the same bootloader, despite not being in the pjrc/teensy4
folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea how that warrants putting it in the SOC folder because it has nothing to do with the SOC
Put it in a common board folder
Added support for the teensy bootloader, allowing teensy_loader_cli to flash the the teensy40 and teensy41 boards without triggering the reset pin. Signed-off-by: Jan Behrens <[email protected]>
Added ability to set teensy_loader_cli flags from teenys runner. Signed-off-by: Jan Behrens <[email protected]>
Added a snippet that includes all Kconfig options and a dt overlay to allow for easy use of the teensy bootloader. Signed-off-by: Jan Behrens <[email protected]>
|
CONFIG_USB_DEVICE_STACK=y | ||
CONFIG_USB_CDC_ACM=y | ||
CONFIG_CDC_ACM_DTE_RATE_CALLBACK_SUPPORT=y | ||
CONFIG_USB_DEVICE_INITIALIZE_AT_BOOT=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about to become deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the new way to do this?
CONFIG_USB_DEVICE_VID=0x16c0 | ||
CONFIG_USB_DEVICE_PID=0x0483 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot use someone else's vendor ID in the tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is, it needs to be there, as the teensy cli looks for this specific VID/PID combination to send the soft-reboot command to.
I honestly have no idea what the process would be for getting permission to use the vendor id for this use case.
There are a handfull of uses of specific vendor ids in the tree, but I dont know their background.
Some other potential solutions, if we can not get permission to use the vendor id, would be to note the correct vendor id in the snippet readme, or look if we can get support for the zephyr vendor id in the teensy cli.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is, it needs to be there, as the teensy cli looks for this specific VID/PID combination to send the soft-reboot command to.
No, it does not need to be there. It is entirely the "teensy cli" problem.
I honestly have no idea what the process would be for getting permission to use the vendor id for this use case.
"Each Vendor ID Number is assigned to one company for its sole and exclusive use, along with associated Product ID Numbers. They may not be sold, transferred, or used by others, directly or indirectly, except in special circumstances and then only upon prior written approval by USB-IF. Unauthorized use of assigned or unassigned USB Vendor ID Numbers and associated Product ID Numbers are strictly prohibited." [0]
Also, I am not going to write about Vendor ID 0x16c0
, but you can find some information about it on your own.
There are a handfull of uses of specific vendor ids in the tree, but I dont know their background.
I see only one left. I will remove it as well.
look if we can get support for the zephyr vendor id in the teensy cli.
It would not help you with your custom firmware/product because of [0]. I guess "teensy cli" could be improved to accept VID/PID arguments, but that is not an issue for the Zephyr Project.
[0] https://www.usb.org/sites/default/files/usb-if_member_agreement_080618_1.pdf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification.
I did some investigation into teensy_cli, but the required changes are more difficult than they first appeared, so I currently don't plan on pursuing that avenue any further.
At this point it looks to me like the best option is to remove the vendor id from the snippet conf file and mention the correct vendor id in the readme instead.
I am also thinking about adding information about the snippet/soft reboot to the flashing section of the board readme.
addressed. consider creating a boards/pjrc/common/ folder as some other vendors have done.
This PR adds the support for the teensy cli tools soft reboot function by adding a bootloader functionality similar to the bossa one to the teensy boards and adding the ability to pass cli tool flags to the teensy runner.
This allows for flashing the board without triggering the programming pin or pressing the reset button.