Add EXTRA_UART cmake option, bridges UART0 over USB-CDC as tty1 #102

milipropasm · 2023-10-01T06:08:15Z

full (probably excessive) details in commit message

- The reset pin must move otherwise uart0 tx is squashed - Don't preempt printf, it doesn't like it - Set up the UART by default

…asis, as opposed to the byte FIFO employed previously. This allows multiple commmands to be framed correctly, so they can be processed sequentially without losing packets. Suspend DAP thread until the end of the USB callback. This prevents the need for continous polling by DAP thread.

Fix ARM CMSIS-DAP issues See merge request projectmu/picoprobe!1

It's possible for the Windows CDC-ACM driver to ignore the IN endpoint for long periods of time - multiple frames - if the host application doesn't consume uart RX data. Boost buffer sizes to compensate. Also prevent usb_thread from potentially being idle for a tick when there's work to do.

CMakeLists.txt added EXTRA_UART option, sets CDC_UARTS board_pico_config.h define PICOPROBE_EX_UART_* if CDC_UARTS > 1 tusb_config.h changed CFG_TUD_CDC from 1 to CDC_UARTS main.c changed UART0 comment change cdc_task() to cdc_tasks() cdc_uart.h change cdc_task() to cdc_tasks() cdc_uart.c add BUF_SIZ defines, tx/rx_buf become 2D arrays: [CDC_UARTS][TX_BUF_SIZ] was_connected and cdc_tx_oe change from function-static variables to module-static[CDC_UARTS] arrays cdc_uart_init() inits new was_connected and cdc_tx_oe arrays. if CDC_UARTS > 1, inits PICOPROBE_EX_UART_INTERFACE uart and gpio cdc_task() gets new tty parameter, uses to lookup uart pointer tty used as first level index into rx/tx_buf, was_connected and cdc_tx_oe arrays all tud_cdc_* calls replaced with tud_cdc_n_*(tty,... variants PICOPROBE_UART_RX/TX_LED restricted to tty 0 only add cdc_tasks() wrapper cdc_thread() now calls cdc_tasks() wrapper tud_cdc_line_coding_cb() now uses tty to lookup uart pointer all tud_cdc_* calls replaced with tud_cdc_n_*(tty,... variant tud_cdc_line_state_cb() PICOPROBE_UART_RX/TX_LED restricted to tty 0 only usb_descriptors.c if CDC_UARTS > 1 increase config descriptor length add two new CDC interfaces & three new endpoints add extra CDC descriptor **** desc_ms_os_20 NOT UPDATED ************************ TESTING (basic) Pi4B host only 5.15.56-v7l+ #1575 SMP THREADED 1, PICOPROBE_DEBUG_PROTOCOL PROTO_DAP_V2 (i.e. defaults), DEBUGPROBE not tested cmake [-DEXTRA_UART=1] .. pico_w printf [and uart_puts], via pico picoprobe, reach host minicom ttyACM0 [and ttyACM1] sudo openocd -f interface/cmsis-dap.cfg -f target/rp2040.cfg -c "adapter speed 5000" -c "program xxx.elf verify reset exit" (success, vanilla .cfgs) THREADED 0 unable to test: host USB-CDC and openocd both timed-out

lurch · 2023-10-01T11:31:27Z

I'm not sure what you've done wrong, but this PR ought to include just commit(s) from yourself, not the commits from P33M and others.

milipropasm · 2023-10-01T12:26:52Z

As someone with approx zero experience of git and exactly zero experience of github, I could keep experimenting til I get it right, but I really don't want to cluster-bomb you with more dodgy pull-requests...

Here are the steps I took for #102: could you or someone else cast an eye over them and tell me which ones need to be removed/modified/replaced?

# on github, fork raspberrypi/picoprobe to milipropasm/picoprobe_tty1 (1)
git clone [email protected]:milipropasm/picoprobe_tty1.git (2)
cd picoprobe_tty1 (3)
git remote add upstream [email protected]:raspberrypi/picoprobe.git (4)
git fetch upstream (5)
git branch develop (6)
git checkout -b extra_uart develop (7)
# make changes, test (8)
# git status or log between each step below (9)
git add CMakeLists.txt include/(star) src/(star) (10)
git commit --file=commit-message (11)
git push -u origin # failed (12)
git push --set-upstream origin extra_uart (13)
git reset (no effect, was hoping to shrink commit message) (14 )
git push (15)
# on github UI, new pull request: (16)
# baserepo:raspberrypi/picoprobe, base:develop (17)
# headrepo:milipropasm/picoprobe_tty1, compare:extra_uart (18)

lurch · 2023-10-01T23:38:01Z

I think what happened is that when you did (2) you got a copy of the repo at the default branch (i.e. master), and when you did (6) it then created a new local branch named develop with the contents of your already-checked-out master branch. I think step (6) should have been git checkout develop (which would have then created a local develop branch with a copy of the upstream develop branch.

If my supposition is correct, you should be able to do:

git rebase upstream/develop
git push -f

and that should then "fix" the commits in this PR.

EDIT: And optionally you could do git branch -D develop to delete your "local develop which is actually a copy of upstream master" branch 😉

lurch · 2023-10-02T07:58:07Z

src/cdc_uart.c

@@ -66,80 +74,102 @@ void cdc_uart_init(void) {
    gpio_set_dir(PICOPROBE_UART_DTR, GPIO_OUT);
    gpio_put(PICOPROBE_UART_DTR, 1);
 #endif
+#if (CDC_UARTS > 1)


Should there be a compile-time error or warning if CDC_UARTS is set to a value larger than 2?

Will add that error once you give me the go/no-go on other two points...
My thinking was that user provides or omits EXTRA_UART , then CDC_UARTS only really settable by configure: 1 by default, 2 if EXTRA_UART present. again thoughts were that separating the boolean from the count allowed future expansion by not excluding additional software uarts bridged to CDC.

lurch · 2023-10-02T08:01:52Z

src/cdc_uart.c

 #ifdef PICOPROBE_UART_TX_LED
-        gpio_put(PICOPROBE_UART_TX_LED, 1);
-        tx_led_debounce = debounce_ticks;
+          gpio_put(PICOPROBE_UART_TX_LED, 1);


I wonder if it might be useful to have e.g. PICOPROBE_EX_UART_TX_LED (etc.) which defaults to PICOPROBE_UART_TX_LED if not defined? 🤔

I wouldn't use it, but happy to add - again let me know your preference

actually, those are debugprobe-only, which conflicts with one of the default UART0 pins (and would be tricky to solder the extra 2 uart tx/rx wires to), so I think its better if I make the picoprobe-only nature of EXTRA_UART explicit in picoprobe_config.h - does this sound OK?

I'll also change all instances of CDC_UARTS > 1 to CDC_UARTS == 2
which makes more sense / is less confusing once the compiler range check you suggested is in place

lurch · 2023-10-02T08:04:32Z

include/board_pico_config.h

 #define PICOPROBE_UART_TX 4
 #define PICOPROBE_UART_RX 5
 #define PICOPROBE_UART_INTERFACE uart1
 #define PICOPROBE_UART_BAUDRATE 115200

+#if (CDC_UARTS > 1)
+// if enabled, always ttyACM(n+1)
+#define PICOPROBE_EX_UART_TX 0


Not immediately obvious what EX stands for here - could be EXternal, EXtra, EXtended, EXpired ? 😉

was originally EXTRA, but just seemed too long - let me know preferred

milipropasm · 2023-10-02T09:03:54Z

And thanks for the pointers on the pull request mis-step, much appreciated. Planning to redo the submission from scratch (with new step 6) as a learning experience

lurch · 2023-10-02T10:24:58Z

I'm not directly involved in this repo, so I was only offering "helpful suggestions", I can't give "concrete answers" - you'll have to rely on @P33M for that (or use your best judgement).
If you did want to add support for DebugProbe too (entirely up to you of course), there are a bunch of Test Points that you might be able to make use of - see https://www.raspberrypi.com/documentation/microcontrollers/debug-probe.html#schematics

milipropasm · 2023-10-02T10:32:15Z

Ah, OK - well the suggestions were helpful, so I'll incorporate this latest thinking and do another pull request without the mis-step.

(As a matter of interest, I've already tried soldering to the debug probe test points to get access to VBUS and VSYS, and hit the problem that there's not enough clearance underneath the board in that cute little case for anything but the finest mod-wire)

thanks again for the help and pointers.

lurch · 2023-10-02T11:11:37Z

Yeah, I guess the expectation is that if you're soldering things to the Debug Probe test-points, you can't expect to keep using the cute little case! 😆

milipropasm · 2023-10-02T11:53:55Z

:-) it's a shame tho: I specifically bought one due to the large number of USB cables that float around my workstation - constantly expecting to drop the end of one onto a live breadboarded pico - but there's no VBUS / VSYS connector on the debugprobe, so unless you're powering the target pico independently, you have no choice but to get the soldering iron out... puzzling.

P33M and others added 9 commits August 15, 2023 11:48

Fix debug prints

d47b308

- The reset pin must move otherwise uart0 tx is squashed - Don't preempt printf, it doesn't like it - Set up the UART by default

probe: hook up reset functionality to DAP commands

0761424

picoprobe version 1.0.3

1586ef0

Merge branch 'fix_debug' into 'master'

d13775e

Fix ARM CMSIS-DAP issues See merge request projectmu/picoprobe!1

cdc_uart: support databits, stopbits and parity setup

2658c2c

cdc_uart: add RTS and DTR pins

721b69c

milipropasm mentioned this pull request Oct 1, 2023

export both pico hardware UARTs via picoprobe? #101

Open

lurch linked an issue Oct 2, 2023 that may be closed by this pull request

export both pico hardware UARTs via picoprobe? #101

Open

lurch reviewed Oct 2, 2023

View reviewed changes

milipropasm closed this Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EXTRA_UART cmake option, bridges UART0 over USB-CDC as tty1 #102

Add EXTRA_UART cmake option, bridges UART0 over USB-CDC as tty1 #102

milipropasm commented Oct 1, 2023

lurch commented Oct 1, 2023

milipropasm commented Oct 1, 2023

lurch commented Oct 1, 2023 •

edited

Loading

lurch Oct 2, 2023

milipropasm Oct 2, 2023 •

edited

Loading

lurch Oct 2, 2023

milipropasm Oct 2, 2023

milipropasm Oct 2, 2023

milipropasm Oct 2, 2023

lurch Oct 2, 2023

milipropasm Oct 2, 2023

milipropasm commented Oct 2, 2023

lurch commented Oct 2, 2023

milipropasm commented Oct 2, 2023

lurch commented Oct 2, 2023 •

edited

Loading

milipropasm commented Oct 2, 2023

Add EXTRA_UART cmake option, bridges UART0 over USB-CDC as tty1 #102

Add EXTRA_UART cmake option, bridges UART0 over USB-CDC as tty1 #102

Conversation

milipropasm commented Oct 1, 2023

lurch commented Oct 1, 2023

milipropasm commented Oct 1, 2023

lurch commented Oct 1, 2023 • edited Loading

lurch Oct 2, 2023

Choose a reason for hiding this comment

milipropasm Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

lurch Oct 2, 2023

Choose a reason for hiding this comment

milipropasm Oct 2, 2023

Choose a reason for hiding this comment

milipropasm Oct 2, 2023

Choose a reason for hiding this comment

milipropasm Oct 2, 2023

Choose a reason for hiding this comment

lurch Oct 2, 2023

Choose a reason for hiding this comment

milipropasm Oct 2, 2023

Choose a reason for hiding this comment

milipropasm commented Oct 2, 2023

lurch commented Oct 2, 2023

milipropasm commented Oct 2, 2023

lurch commented Oct 2, 2023 • edited Loading

milipropasm commented Oct 2, 2023

lurch commented Oct 1, 2023 •

edited

Loading

milipropasm Oct 2, 2023 •

edited

Loading

lurch commented Oct 2, 2023 •

edited

Loading