[patch #10154] Update to serial code management for custom connection parameters

[Dawid Buchwald]

Follow-up Comment #8, patch #10154 (project avrdude):

[comment #5 comment #5:]
> 
> [comment #3 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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?10154>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/