Skip to content
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

Certain terminal commands may be confusing #1570

Closed
MCUdude opened this issue Nov 14, 2023 · 7 comments · Fixed by #1574
Closed

Certain terminal commands may be confusing #1570

MCUdude opened this issue Nov 14, 2023 · 7 comments · Fixed by #1574
Labels
enhancement New feature or request

Comments

@MCUdude
Copy link
Collaborator

MCUdude commented Nov 14, 2023

When testing PR #1566 I used my STK500 board(s) in interactive terminal mode to change various settings. To see which board settings I could change at runtime, I ran the parms command:

avrdude: processing -t interactive terminal
avrdude> parms
Vtarget         : 5.1 V
Varef           : 3.1 V
Oscillator      : 7.372800 MHz
SCK period      : 0.6 us
XTAL frequency  : 14.745600 MHz

All good! Now I want to change the oscillator speed. oscillator=1M doesn't work, and neither does oscillator 1M, osc=1M or osc 1M. The syntax is very strict, and I'll have to run ? to list all available commands to find out that it was fosc <value>[M|k] I should have used all along.

  • It's a bit confusing that we can't use the equal sign here (fosc=1M) when -x would require it. Maybe terminal mode could support both?
  • It would be great if we could read the current state using fosc just by running the fosc command without any parameters, or perhaps with a question mark at the end (fosc?)?
  • It would be neat if we at least could print the associated command when printing all customizable parameters using parms.

Any thoughts?

@MCUdude MCUdude added the enhancement New feature or request label Nov 14, 2023
@stefanrueger
Copy link
Collaborator

help tells us what the terminal can and cannot do. It is vvv hard to second guess what people might or might not expect.

I agree that parms could tell the user how parameters can be changed (or whether they can only be changed with a soldering iron, eg, by saying they are a solder-time constant). We should also be mindful when the avrdude user is a script that wants to process the output, eg, as in avrdude -T parms | grep XTAL | cut -f2 -d:. Changes in what and how we display ought to be not to radical, so that scripts don't need too much rewriting. Therefore I'd suggest that the terminal command parms -v adds a column with how to change that value. Note that this -v would be processed only by the parms terminal command and not by avrdude (so it's not the verbose level that needs checking).

Maybe terminal mode could support both?

That is a bit difficult as it requires a radical change in how terminal commands are parsed.

@Ho-Ro
Copy link
Contributor

Ho-Ro commented Nov 14, 2023

oscillator=1M doesn't work, and neither does oscillator 1M, osc=1M or osc 1M

oscillator could be an alias for fosc, just duplicate the fosc line in struct command cmd[] and replace fosc with oscillator -> oscillator 1M as well as osc 1M works.

It's a bit confusing that we can't use the equal sign here (fosc=1M) when -x would require it. Maybe terminal mode could support both?

An evil hack could be to replace in process_line every = by unless the line starts with !, or do this in tokenize, didn't deep-dive enough to test it - this stuff looks to fragile.
Just my 2 ct.

It would be great if we could read the current state using fosc just by running the fosc command without any parameters, or perhaps with a question mark at the end (fosc?)?

Contrary to set_fosc() there's no get_fosc() available for the individual backends, the -xfosc output is calculated and displayed directly out of the backend code.

EDIT: @stefanrueger has typed faster than me

We should also be mindful when the avrdude user is a script that wants to process the output, eg, as in avrdude -T parms | grep XTAL | cut -f2 -d:.

good point

@stefanrueger
Copy link
Collaborator

An evil hack could be to replace in process_line every = by unless the line starts with !

There are ways, but this is probably not a good one. = can be a legitimate character as in write eeprom 0 "1 + 1 = 2". FOr the same reason making = a separation character in tokenize() is bound to end in a headache one way or another. Maybe someone can build a Chat-GPT interface or otherwise to translate human expectation to AVRDUDE syntax 😉

Contrary to set_fosc() there's no get_fosc() available for the individual backends

Great point; that would be a useful libavrdude function.

@Ho-Ro
Copy link
Contributor

Ho-Ro commented Nov 15, 2023

Contrary to set_fosc() there's no get_fosc() available for the individual backends

Great point; that would be a useful libavrdude function.

I identified at least four setter w/o getter candidates:

pgm.c:

  pgm->set_vtarget    = NULL;
  pgm->set_varef      = NULL;
  pgm->set_fosc       = NULL;
  pgm->set_sck_period = NULL;

libavrdude.h:

  int  (*set_vtarget)    (const struct programmer_t *pgm, double v);
  int  (*set_varef)      (const struct programmer_t *pgm, unsigned int chan, double v);
  int  (*set_fosc)       (const struct programmer_t *pgm, double v);
  int  (*set_sck_period) (const struct programmer_t *pgm, double v);

Global implementation would look like:

  pgm->get_vtarget    = NULL;
  int  (*get_vtarget)    (const struct programmer_t *pgm, double *v);

with optional individual backend functions enabled by HAS_VTARG_ADJ or HAS_VTARG_READ.

Ho-Ro added a commit to Ho-Ro/avrdude that referenced this issue Nov 17, 2023
…sck_period

this is only a proof of concept
declarations for libavrdude, parser in term.c
sample implementations in stk500v1.c

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>
@Ho-Ro
Copy link
Contributor

Ho-Ro commented Nov 17, 2023

I provided a proof of concept for the 4 missing libavrdude functions.
If you like you can try my get_4_1570, that was branched from current main branch.

@stefanrueger
Copy link
Collaborator

@Ho-Ro Thanks for this proposal; best if @mcuee and @MCUdude test this. The code looks OK (but I haven't done a proper review yet).

@mcuee
Copy link
Collaborator

mcuee commented Nov 18, 2023

@Ho-Ro Thanks for this proposal; best if @mcuee and @MCUdude test this. The code looks OK (but I haven't done a proper review yet).

@Ho-Ro
In this case, it is better to create a PR so that formal review and testing can be done. Thanks.
You can mark it as WIP if you think it is not ready yet.

Ho-Ro added a commit to Ho-Ro/avrdude that referenced this issue Nov 18, 2023
…sck_period

this is only a proof of concept
declarations for libavrdude, parser in term.c
sample implementations in stk500v1.c

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>
Ho-Ro added a commit to Ho-Ro/avrdude that referenced this issue Nov 22, 2023
…sck_period

this is only a proof of concept
declarations for libavrdude, parser in term.c
sample implementations in stk500v1.c

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>
Ho-Ro added a commit to Ho-Ro/avrdude that referenced this issue Nov 22, 2023
…sck_period

this is only a proof of concept
declarations for libavrdude, parser in term.c
sample implementations in stk500v1.c

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>
Ho-Ro added a commit to Ho-Ro/avrdude that referenced this issue Nov 26, 2023
…sck_period

this is only a proof of concept
declarations for libavrdude, parser in term.c
sample implementations in stk500v1.c

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>
stefanrueger added a commit that referenced this issue Dec 1, 2023
new functions (#1570): get_vtarget, get_varef, get_fosc, get_sck_period
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants