Put static variables into context structure #1803

stefanrueger · 2024-05-06T21:53:42Z

This PR intends to move all static variables used in libavrdude functions into a single structure called cx_t; the idea is that there is one global context pointer cx_t *cx that libavrdude applications need to allocate memory for at the beginning of each instantiation. Ideally, one would hide the components of the context structure and write access function that are used in libavrdude functions. However, that ship has sailed given all the explicit PROGRAMMER, AVRPART and AVRMEM structures and the tons of existing direct -> access to their components. And more importantly, there are a lot of variables to convert (some 111), so writing 222 access functions, one each for get and set, wouldn't be much fun.

@dl8dtl @MCUdude @mcuee Have a look at the commit for the first source file that has been treated that way, avr.c, and see what you think. Other than that: it's one down and 34 to do.

... and convert static memory for strings returned by avr_prog_modes() into closed-circuit memory area in the context structure

mcuee · 2024-05-07T02:07:26Z

@stefanrueger

I am not so sure if I understand the intention correctly, but this seems to be able to enable multiple context with libavrdude. In that case, it may be good to have a function of libavrdude_init(&context) and libavrdude_exit(contex).

Reference from libusb.
https://libusb.sourceforge.io/api-1.0/group__libusb__lib.html

int libusb_init_context	(	
libusb_context ** ctx,
const struct libusb_init_option options[],
int 	num_options 
)	

void libusb_exit(libusb_context * ctx)

stefanrueger · 2024-05-07T20:54:19Z

libavrdude_init(&context)

Good shout, I wanted to work through the needed variables first and see which ones actually need initialisation.

The latter returns a string in a static buffer that is overwritten by the next call. Unfortunately, this function was used more than once in arguments of the *same* msg_error(), which would have given a wrong error message. pins_to_strdup() returns a string to malloc'd space that needs freeing.

... as it was used wrongly and led to errors. Better to use pin_to_strdup()

No longer is the string returned in static space (which the next call overwrites); instead a mmt_strdup()'d string is returned that needs mmt_free()'ing.

stefanrueger · 2024-05-10T21:39:02Z

As far as I am aware all static variables are now dealt with and moved to the context structure. Following simple test works:

$ yes "" | tools/test-avrdude -Td0

With the exception of one variable (usb_buflen) all were initialised to 0, and I think(!) usb_buflen can live with also being initialised to 0. So, @mcuee looks like we don't need an init() function.

It's quite possible that something went wrong when I migrated the variables, so a few functionality checks with hardware will be needed. @mcuee? @MCUdude?

I suggest merging a batch of current PRs including this one early next week, so that @dl8dtl can progress his PR #1714 (as this PR is meant to make libavrdude ready for new applications such as his GDI, in particular ones that allow various different programming sessions without leaving the application).

mcuee · 2024-05-11T23:53:25Z

Okay, let me carry out some simple tests in the next few days.

mcuee · 2024-05-13T02:55:28Z

First test: the results are good.

MINGW64 /c/work/avr/avrdude_test/avrdude_sr
$ ./tools/test-avrdude -e ./build_mingw64/src/avrdude.exe -p "-c usbasp -p m328p"
Testing ./build_mingw64/src/avrdude.exe version 7.3-20240510 (627f320e)
Prepare "-c usbasp -p m328p" and press 'enter' or 'space' to continue. Press any other key to skip
✅   0.592 s: fuse access: clear, set and read eesave fuse bit
✅   0.519 s: fuse access: set eesave fusebit to delete EEPROM on chip erase
✅   0.788 s: chip erase
✅   2.730 s: flash -U write/verify holes_rjmp_loops_32768B.hex
✅   1.260 s: flash -T write/verify holes_rjmp_loops_32768B.hex
✅   0.620 s: eeprom check whether programmer can flip 0s to 1s
✅   3.247 s: eeprom -U write/verify holes_pack_my_box_1024B.hex
✅   6.143 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_1024B.hex
✅   1.468 s: chip erase and spot check flash is actually erased
✅   0.611 s: spot check eeprom is erased, too

mcuee · 2024-05-13T05:16:53Z

Second test is also fine.

MINGW64 /c/work/avr/avrdude_test/avrdude_sr
$ ./tools/test-avrdude -e ./build_mingw64/src/avrdude.exe -p "-c urclock -P COM4 -p m328p"
Testing ./build_mingw64/src/avrdude.exe version 7.3-20240510 (627f320e)
Prepare "-c urclock -P COM4 -p m328p" and press 'enter' or 'space' to continue. Press any other key to skip
✅   1.770 s: chip erase
✅   3.251 s: flash -U write/verify holes_rjmp_loops_32768B.hex
✅   1.185 s: flash -T write/verify holes_rjmp_loops_32768B.hex
✅   0.617 s: eeprom check whether programmer can flip 0s to 1s
✅   1.619 s: eeprom -U write/verify holes_pack_my_box_1024B.hex
✅   2.893 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_1024B.hex
✅   2.319 s: chip erase and spot check flash is actually erased

mcuee · 2024-05-13T05:28:07Z

I have not tested the legacy Arduino optiboot bootloader for a while and it is okay. The failure is expected as the bootloader does not support EEPROM and chip erase.

MINGW64 /c/work/avr/avrdude_test/avrdude_sr

$ ./build_mingw64/src/avrdude.exe -c urclock -P COM4 -p m328p -xshowall
avrdude: AVR device initialized and ready to accept instructions
0 0000-00-00 00.00  application 0 store 0 meta 1 boot 512 o4.4 --s-h-r-- vector 0 (RESET) ATmega328P

avrdude done.  Thank you.

$ ./tools/test-avrdude -e ./build_mingw64/src/avrdude.exe -p "-c arduino -P COM4 -p m328p"
Testing ./build_mingw64/src/avrdude.exe version 7.3-20240510 (627f320e)
Prepare "-c arduino -P COM4 -p m328p" and press 'enter' or 'space' to continue. Press any other key to skip
✅   1.569 s: chip erase
✅   3.181 s: flash -U write/verify holes_rjmp_loops_32768B.hex
✅   2.168 s: flash -T write/verify holes_rjmp_loops_32768B.hex
✅   1.535 s: eeprom check whether programmer can flip 0s to 1s
❌   2.621 s: eeprom -U write/verify holes_pack_my_box_1024B.hex (failed command below)
$ ./build_mingw64/src/avrdude.exe -qq -c arduino -P COM4 -p m328p -Ueeprom:w:./tools/test_files/holes_pack_my_box_1024B.hex
❌   6.065 s: eeprom -U write/verify the_quick_brown_fox_1024B.hex (failed command below)
$ ./build_mingw64/src/avrdude.exe -qq -c arduino -P COM4 -p m328p -Ueeprom:w:./tools/test_files/the_quick_brown_fox_1024B.hex
✅   4.010 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_1024B.hex
❌   2.241 s: chip erase and spot check flash is actually erased (failed command below)
$ ./build_mingw64/src/avrdude.exe -qq -c arduino -P COM4 -p m328p -e -FAU flash:w:0xff:m -U flash:v:./tools/test_files/holes_flash_0xff_32768B.hex

One or more AVRDUDE "-c arduino -P COM4 -p m328p" tests failed. Do you want to retry this particular test? (y/n): n

$ ./tools/test-avrdude -e ./build_mingw64/src/avrdude.exe -p "-c urclock -P COM4 -p m328p"
Testing ./build_mingw64/src/avrdude.exe version 7.3-20240510 (627f320e)
Prepare "-c urclock -P COM4 -p m328p" and press 'enter' or 'space' to continue. Press any other key to skip
✅  10.031 s: chip erase
✅  10.084 s: flash -U write/verify holes_rjmp_loops_32768B.hex
✅   1.824 s: flash -T write/verify holes_rjmp_loops_32768B.hex
❌   1.100 s: eeprom check whether programmer can flip 0s to 1s (failed command below)
$ ./build_mingw64/src/avrdude.exe -qq -c urclock -P COM4 -p m328p -Ueeprom:w:0x55:m -Ueeprom:w:0xaa:m
# ... the next test may therefore take longer
❌   1.155 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_1024B.hex (failed command below)
$ ./build_mingw64/src/avrdude.exe -qq -c urclock -P COM4 -p m328p -T "write eeprom ./tools/test_files/holes_the_five_boxing_wizards_1024B.hex:a" -T flush -T "write eeprom ./tools/test_files/holes_pack_my_box_1024B.hex:a"
❌   1.251 s: eeprom -T write/verify lorem_ipsum_1024B.srec (failed command below)
$ ./build_mingw64/src/avrdude.exe -qq -c urclock -P COM4 -p m328p -T "write eeprom ./tools/test_files/lorem_ipsum_1024B.srec:a"
✅  10.810 s: chip erase and spot check flash is actually erased

One or more AVRDUDE "-c urclock -P COM4 -p m328p" tests failed. Do you want to retry this particular test? (y/n):

stefanrueger · 2024-05-13T22:57:21Z

@mcuee Thanks for testing. And I can contribute two programmers as well:

$ ./tools/test-avrdude -p "-c usbtiny -p m2560 -B 4MHz" -d 0
Testing avrdude version 7.3-20240510 (627f320e)
Prepare "-c usbtiny -p m2560 -B4MHz" and press 'enter' or 'space' to continue. Press any other key to skip
✅   0.229 s: fuse access: clear, set and read eesave fuse bit
✅   0.110 s: fuse access: set eesave fusebit to delete EEPROM on chip erase
✅   0.421 s: chip erase
✅  50.063 s: flash -U write/verify holes_rjmp_loops_262144B.hex
✅  31.497 s: flash -T write/verify holes_rjmp_loops_262144B.hex
✅   1.096 s: eeprom check whether programmer can flip 0s to 1s
✅   4.591 s: eeprom -U write/verify holes_pack_my_box_4096B.hex
✅   9.512 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_4096B.hex
✅  32.042 s: chip erase and spot check flash is actually erased
✅   0.836 s: spot check eeprom is erased, too

$ ./tools/test-avrdude -p "-c usb-bub-ii -p m328p" -d 0
Testing avrdude version 7.3-20240510 (627f320e)
Prepare "-c usb-bub-ii -p m328p" and press 'enter' or 'space' to continue. Press any other key to skip
✅   0.220 s: fuse access: clear, set and read eesave fuse bit
✅   0.098 s: fuse access: set eesave fusebit to delete EEPROM on chip erase
✅   0.344 s: chip erase
✅   6.757 s: flash -U write/verify holes_rjmp_loops_32768B.hex
✅   3.281 s: flash -T write/verify holes_rjmp_loops_32768B.hex
✅   0.237 s: eeprom check whether programmer can flip 0s to 1s
✅   1.871 s: eeprom -U write/verify holes_pack_my_box_1024B.hex
✅   3.774 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_1024B.hex
✅   3.540 s: chip erase and spot check flash is actually erased
✅   0.462 s: spot check eeprom is erased, too

The latter being a ftdi_syncbb-type programmer rigged up with own cabelling defined in ~/.avrduderc

MCUdude · 2024-05-14T06:29:29Z

PR looks good to me! Are there any specific programmers you want me to test with?

stefanrueger · 2024-05-14T12:19:32Z

PR looks good to me! Are there any specific programmers you want me to test with?

@MCUdude Thanks! If you have, an avrftdi-type programmer test would be great, particularly one where you copy the programmer entry into your private ~/.avrduderc and assign a wrong pin number to one of the signals so that avrftdi_check_pins_mpsse() triggers an error message.

Also bitbanging programmers (serbb type) or linuxgpio had a bit of a change...

ndim · 2024-05-14T16:12:35Z

Regarding cx_t... have you considered that POSIX reserves all type names ending in _t?

Answering my own question: The avrdude code is full of other foo_t types, so adding one more should not make much of a difference.

stefanrueger · 2024-05-14T18:17:18Z

@ndim Great shout. I'll change cx_t to sth else and, at another time, think about the 39 other foo_t that AVRDUDE uses:

$ cat $(find /usr/include/ -type f) | tr -c a-zA-Z_0-9 \\n | grep _t$ | sort -u >/tmp/p
$ grep -hr _t src | tr -c a-zA-Z_0-9 \\n | grep _t$ | sort -u  | comm -32 - /tmp/p
avrftdi_t
Blhash_t
CacheDesc_t
Cfg_opts_t
Cfg_t
Component_t
Configitem_t
conntype_t
ctl_stack_t
dry_prog_t
dryrun_t
exit_datahigh_t
exit_reset_t
exit_vcc_t
Flock_t
fl_t
Fusel_t
is_outside_int64_t
leds_t
memtable_t
memtype_t
pdata_t
pin_checklist_t
pindef_t
pinmask_t
ppipins_t
programmer_t
programmer_type_t
Register_file_t
sckoptions_t
Segment_t
Segorder_t
token_t
ulong_t
uPcore_t
update_t
Urclock_t
Valueitem_t
Write_mode_t

Maybe, we can replace xyx_t to Xyz? Just thinking aloud.

mcuee · 2024-05-16T23:44:38Z

@ndim Great shout. I'll change cx_t to sth else and, at another time, think about the 39 other foo_t that AVRDUDE uses:

Maybe, we can replace xyx_t to Xyz? Just thinking aloud.

I think this is a good idea (some of them is already Xyz_t but should still be okay).
Or maybe we can replace xyz_t with xyz_type.

BTW, changing cx_t to libavrdude_context is certainly a welcome one.

mcuee · 2024-05-17T04:18:46Z

Also bitbanging programmers (serbb type) or linuxgpio had a bit of a change...

No issues with linuxgpio.

mcuee@raspberrypi400arm64:~/build/avrdude_sr $ 
./tools/test-avrdude -e ./build_linux/src/avrdude -p "-c linuxgpio -p m328p"
Testing ./build_linux/src/avrdude version 7.3-20240514 (39336422)
Prepare "-c linuxgpio -p m328p" and press 'enter' or 'space' to continue. Press any other key to skip
✅   0.340 s: fuse access: clear, set and read eesave fuse bit
✅   0.328 s: fuse access: set eesave fusebit to delete EEPROM on chip erase
✅   0.489 s: chip erase
✅   1.845 s: flash -U write/verify holes_rjmp_loops_32768B.hex
✅   1.451 s: flash -T write/verify holes_rjmp_loops_32768B.hex
✅   0.321 s: eeprom check whether programmer can flip 0s to 1s
✅   1.334 s: eeprom -U write/verify holes_pack_my_box_1024B.hex
✅   2.111 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_1024B.hex
✅   1.042 s: chip erase and spot check flash is actually erased
✅   0.353 s: spot check eeprom is erased, too

mcuee@raspberrypi400arm64:~/build/avrdude_sr $ cat ~/.avrduderc 
# #------------------------------------------------------------
# # linuxgpio
# #------------------------------------------------------------
#
programmer
    id                   = "linuxgpio";
    desc                 = "Use the Linux libgpiod interface to bitbang GPIO lines";
    type                 = "linuxgpio";
    prog_modes           = PM_ISP;
    connection_type      = linuxgpio;
    reset                = 25;
    sck                  = 11;
    sdo                  = 10;
    sdi                  = 9;
;

Put static variables of avr.c into context structure

9f69a6e

... and convert static memory for strings returned by avr_prog_modes() into closed-circuit memory area in the context structure

stefanrueger linked an issue May 6, 2024 that may be closed by this pull request

0xff optimization when reading #1732

Closed

mcuee added the enhancement New feature or request label May 7, 2024

Put static variables of avrpart.c into context structure

fa186e2

Put static variables of bitbang.c into context structure

8a36a2f

stefanrueger force-pushed the context branch from 12a789d to 8a36a2f Compare May 7, 2024 21:25

stefanrueger added 3 commits May 7, 2024 22:35

Make static variable const in avrftdi_tpi.c

1b0fb27

Put static variables of config.c into context structure

14b337b

Remove static variable in winconf.c

d425e73

stefanrueger force-pushed the context branch from 608216c to d425e73 Compare May 8, 2024 18:58

stefanrueger added 18 commits May 8, 2024 22:10

Consolidate functions returning a prog_modes string

161f763

Make two static structs read-only in developer_opts.c

0c56535

Put static variable of dfu.c into context structure

6d8fce5

Guard against out-of-range pin_name in config_gram.y

b25c28a

Add author to config_gram.y for contributions from 2022

fb1a63b

Put static variable of config_gram.y into context structure

c2264ad

Make static function greppable in fileio.c

bf414c7

Remove unused function pin_to_str()

c5c49d7

... as it was used wrongly and led to errors. Better to use pin_to_strdup()

Change pinmask_to_str() to pinmask_to_strdup()

4ccbf49

No longer is the string returned in static space (which the next call overwrites); instead a mmt_strdup()'d string is returned that needs mmt_free()'ing.

Change free() to mmt_free()

9431330

Put static variable of ppi.c into context structure

292947d

Put static variables of ser_avrdoper.c into context structure

8025107

Harden avrdude against early pmsg_...() calls

5dc019c

Provide avr_cc_buffer(n) for allocating n bytes in temporary buffer

9d15a16

Utilise is_memset() in avrcache/developer_opts

e85ecfa

Provide safety margin for avr_cc_buffer(n)

3bbb093

Use avr_cc_buffer() instead of static buffer for returning a string

9fb2f42

stefanrueger added 13 commits May 10, 2024 19:28

Revert to pin_to_str() and pinmask_to_str()

a22f2d8

Change static buffer to closed-circuit buffer in term.c

7936f00

Rename last and done variables

531b00d

Rename static int i and static char *header

fff80ea

Rename static read_mem

a97f0f8

Put static variables of term.c in context structure

adb4fef

Rename static update.c variables

9af06c7

Put static variables of update.c into context structure

3a45e1f

Make static function greppable in usb_libusb.c

8e62551

Rename usb_libusb.c variables

b4f5851

Put static variables of usb_libusb.c into context structure

512c3b6

Make static array const in serbb_posix.c

cca61bf

Make static array const in flip1.c

627f320

stefanrueger changed the title ~~WIP: put static variables into context structure~~ Put static variables into context structure May 10, 2024

stefanrueger mentioned this pull request May 13, 2024

Add serprog programmer #1801

Merged

Rename cx_t to libavrdude_context

3933642

stefanrueger merged commit 9d02c37 into avrdudes:main May 17, 2024
13 checks passed

stefanrueger deleted the context branch May 17, 2024 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put static variables into context structure #1803

Put static variables into context structure #1803

stefanrueger commented May 6, 2024

mcuee commented May 7, 2024 •

edited

stefanrueger commented May 7, 2024

stefanrueger commented May 10, 2024

mcuee commented May 11, 2024

mcuee commented May 13, 2024

mcuee commented May 13, 2024

mcuee commented May 13, 2024

stefanrueger commented May 13, 2024

MCUdude commented May 14, 2024

stefanrueger commented May 14, 2024

ndim commented May 14, 2024 •

edited

stefanrueger commented May 14, 2024

mcuee commented May 16, 2024 •

edited

mcuee commented May 17, 2024

Put static variables into context structure #1803

Put static variables into context structure #1803

Conversation

stefanrueger commented May 6, 2024

mcuee commented May 7, 2024 • edited

stefanrueger commented May 7, 2024

stefanrueger commented May 10, 2024

mcuee commented May 11, 2024

mcuee commented May 13, 2024

mcuee commented May 13, 2024

mcuee commented May 13, 2024

stefanrueger commented May 13, 2024

MCUdude commented May 14, 2024

stefanrueger commented May 14, 2024

ndim commented May 14, 2024 • edited

stefanrueger commented May 14, 2024

mcuee commented May 16, 2024 • edited

mcuee commented May 17, 2024

mcuee commented May 7, 2024 •

edited

ndim commented May 14, 2024 •

edited

mcuee commented May 16, 2024 •

edited