patchAVR Downloader/UploaDEr - Patches: patch #10154, Update to serial code management...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

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

Submitter:  Dawid Buchwald <dawidbuchwald>
Submitted:  Wed 01 Dec 2021 03:47:48 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  In Progress Privacy:  Public
Assigned to:  None Open/Closed:  Open

Jump to the original submission

Sat 04 Dec 2021 07:55:47 PM UTC, comment #9: 

OK, you're correct, union pinfo is not initialized.

Dragging all that into pgm is probably not such a good idea, not sure.

Nevertheless, I'd vote for some simplification in the cflags handling, as we'll for sure never use all combinations of the cflags bits, so why move all these details out from the serial code into the programmer implementations (even if its only done by a DEFAULT macro)?

Should we ever need more than 8N1 and 8E2, we could simply add that.

Joerg Wunsch <joerg_wunsch>
Group administrator
Sat 04 Dec 2021 07:38:29 PM UTC, comment #8: 

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.

Dawid Buchwald <dawidbuchwald>
Group Member
Sat 04 Dec 2021 07:27:08 PM UTC, comment #7: 

comment #6:

>
> We could rename it to "setparms" then, meaning it sets both the speed as well as the cflags.
>
> Does that make sense to you?
>


Yeah, definitely, I think this change makes a lot of sense.

Dawid Buchwald <dawidbuchwald>
Group Member
Sat 04 Dec 2021 05:15:16 PM UTC, comment #6: 


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?

Joerg Wunsch <joerg_wunsch>
Group administrator
Sat 04 Dec 2021 05:11:11 PM UTC, 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.

> > 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.

> > 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.

Joerg Wunsch <joerg_wunsch>
Group administrator
Sat 04 Dec 2021 01:18:04 PM UTC, comment #4: 

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.

Dawid Buchwald <dawidbuchwald>
Group Member
Sat 04 Dec 2021 01:14:02 PM UTC, comment #3: 

comment #1:

> I wonder whether we could get around those
>
> pinfo.serialinfo.cflags = SERIAL_DEFAULT_FLAGS;
>
> everywhere.
>


Well, not really, the point is to explicitly state what do we want. Depending on where in memory the variable is defined and how it gets (or doesn't) initialised is not very safe, and safety I was going for, even if at the cost of verbosity.

> Couldn't we just leave that as 0 (implied by zeroing out the pgm struct initially), and then, in the serial drivers, handle 0 as the classic "raw" mode flags ("8N1")?


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.

>
> What alternate cflags settings does SerialUPDI require?
>


Even parity enabled, two stop bits.

> 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.

Dawid Buchwald <dawidbuchwald>
Group Member
Sat 04 Dec 2021 12:19:57 AM UTC, 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).

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.

David Sainty <dsainty>
Fri 03 Dec 2021 10:10:45 PM UTC, comment #1: 

I wonder whether we could get around those

pinfo.serialinfo.cflags = SERIAL_DEFAULT_FLAGS;

everywhere.

Couldn't we just leave that as 0 (implied by zeroing out the pgm struct initially), and then, in the serial drivers, handle 0 as the classic "raw" mode flags ("8N1")?

What alternate cflags settings does SerialUPDI require?

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.

Joerg Wunsch <joerg_wunsch>
Group administrator
Wed 01 Dec 2021 03:47:48 PM UTC, original submission:  

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.

Dawid Buchwald <dawidbuchwald>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #52398:  serial_params.patch added by dawidbuchwald (15KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dsainty (Posted a comment)
  • -email is unavailable- added by joerg_wunsch (Posted a comment)
  • -email is unavailable- added by dawidbuchwald (Submitted the item)
  • -email is unavailable- added by dawidbuchwald
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-12-03 joerg_wunsch StatusNone In Progress
    2021-12-01 dawidbuchwald Attached File- Added serial_params.patch, #52398
        Carbon-Copy- Added dawidbuchwald

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code