comment #5:

>
> comment #3:
>
> > If you do use some mechanism to ensure that the default value will always be 0, then sure. Thing is, AFAIK, without proper constructor you can guarantee that.
>
> There is a proper constructor, pgm_new() (in pgm.c), and it ensures the entire pgm object is zeroed out initially.
>
> I seem to remember we rely on that already elsewhere.
>
> So yes, this is safe.
>

Well, sorry, but I disagree. This is how the union pinfo is declared in most places where it's used:

static int buspirate_open(struct programmer_t *pgm, char * port)
{
        union pinfo pinfo;
        /* BusPirate runs at 115200 by default */
        if(pgm->baudrate == 0)
                pgm->baudrate = 115200;

        pinfo.baud = pgm->baudrate;

So yeah, in that particular place you can assume that pgm is initialized properly, but union pinfo? That's allocated on the stack and can contain any possible value.

Maybe I'm not reading your idea correctly, maybe you are suggesting to move serial port settings to pgm structure - in that case it could be assumed that the pgm_new() constructor will handle all the initialisation details.

> > > What alternate cflags settings does SerialUPDI require?
> > >
> >
> > Even parity enabled, two stop bits.
>
> I guess it's simpler then to just create an enum with the desired setting variants, like SER_8N1, SER_8E2 and so on. That avoids having to create AVRDUDE-specific versions for each individual cflag which the driver then needs to remap to each particular system's cflag values anyway.
>

This is why I took care of all the standard options (parity/bits/stopbits). There are not many others.

> > > wAVR also wants to make the timeouts more flexible (since network timeouts can easily exceed 100 ms), maybe we need a kind of architectural overhaul for all of this.
> >
> > That is also something that could be considered. I was looking for correct (and safe!) method that would have as little impact on the code as possible.
>
> Since we are going to restructure things, it makes sense to also cleanup some historical stuff.
>
> That's by no means to blame you on that, it's simply that many things in AVRDUDE are "historically grown"m rather than designed the way they are now.

Yeah, I don't take it as blame, and please understand - I don't want to defend my implementation. You know this code much better, and at the end of the day it should be your call how to progress. Should you decide to implement all that differently, I will be happy to simply take the new version and base my implementation on top of that.

If you would rather have me redo the changes following your guidance I will also be very happy to do so. It's your project and I'm here just to help out. I don't want to impose or force any ideas here, and I want you to know that I deeply respect your effort. AVRDUDE is awesome software that I really love to use, it made my venture into world of microcontrollers much easier, and I'm deeply grateful for that.

That all being said - let me know how you would like to progress, I will follow your lead on this.

comment #4:

> If anything I would ask why serial connection parameters (like number of databits and parity) are set in a function called setspeed().

We could rename it to "setparms" then, meaning it sets both the speed as well as the cflags.

Does that make sense to you?

comment #2:

> It also looks a bit abusive to add flags to the setspeed() method.  If there was a new setcflags() method then it could just be not called for all drives that don't need to change the cflags, and none of the drivers would need to change at all (default would apply if setcflags() is not called).

Well, to be fair, the problem is not new. If anything I would ask why serial connection parameters (like number of databits and parity) are set in a function called setspeed(). So yeah, I agree that it should probably be "architecturally overhauled", but that would have to happen at the risk of breaking compatibility with existing programmers. I don't have necessary resources to ensure required quality, sorry.

>
> On a tangent - if thinking about overhauls, getting rid of the serdev (and serial_recv_timeout) globals would be nice.
>

As long as you can propose any other way to still being able to use them - in my code I also need to alter timeout.

It also looks a bit abusive to add flags to the setspeed() method.  If there was a new setcflags() method then it could just be not called for all drives that don't need to change the cflags, and none of the drivers would need to change at all (default would apply if setcflags() is not called).

On a tangent - if thinking about overhauls, getting rid of the serdev (and serial_recv_timeout) globals would be nice.

The library relies on the serdev type matching the `union file descriptor *fd` that is passed around, but an object oriented approach would just have *fd knowing what type it was.

As described in an e-mail sent to AVRDUDE developers mailing list, the purpose of this patch is to enable custom connection parameters for serial connection. This will be required by upcoming SerialUPDI driver.