Improve upload/program logic

MCUdude commented 4 years ago

PlatformIO is just excellent, and I always use it when working on larger projects. Now that Mighty/Mini/Mega/Major/MicroCore is implemented and works excellent with the fuses.py and bootloader.py scripts, it's a VERY decent platform for serious development.

There's one thing that's slightly annoying though, that I'd love to improve or see being improved. It's the upload/program commands and what they do. Before I continue I'll like to point out that I'll love to dive into this myself, if you (@valeros and maybe @ivankravets) will accept a PR with a fix/improvement. I'll just need a little advice on how and where to start.

Let's say you have a project where you use PlatformIO to calculate + set the fuse bits, load the bootloader and uploading using the bootloader on a bunch of targets. At the moment you can't do this without modifying platformio.ini for each step. The commands for this would look like this:

$ pio run -t bootloader
...
$ pio run -t upload

The problem is that you can't upload using a USB to serial adapter when upload_protocol is defined in platformio.ini. upload_protocol will override the serial upload part.

Another this is that if you have specified a programmer for uploading and you'll accidentally click the "regular" upload button is VScode, the underlying script will add the -D flag to the Avrdude command, and uploading will fail. If you instead run pio run -t program, the Avrdude command is identical, apart from not having the -D flag.

Here's an idea that will work for Mighty/Mini/Mega/Major/MicroCore since these cores have extra hardware parameters:

board_hardware.uart = uart0..3 is present in platformio.ini. This means that the user wants to use the bootloader for uploads.
- Override upload_protocol of the present in platformio.ini when running the pio run -t upload command, and set it to upload_protocol = arduino to prepare for bootloader upload. Use the defined upload_port and force the -D flag. Ignore the upload flags in this case
- If upload_protocol is present and the user runs pio run -t fuses or pio run -t bootloader, use the upload_protocol and the upload_flags fields, just like it's done today. If upload_protocol isn't present, throw an error.
board_hardware.uart = no_bootloader is present in platformio.ini, or the target simply doesn't support serial uploads (ATtiny13, ATmega48/P). This means that the user wants to use the programmer for uploads.
- Use the upload_protocol and upload_flags when running pio run -t fuses, pio run -t bootloader or pio run -t upload. Make sure the -D flag isn't there, as it will mess up the upload procedure.

This means that there's no use for the pio run -t program command anymore, since the upload commands will be "calculated" based on the content in the ini file.

I hope my writing is understandable to you. The goal here is that you could have a platformio.ini file pretty much written in stone, and you can set fuses/burn bootloader and upload using a USB to serial adapter without having to change anything in the platformio.ini file. Any thoughts? Do you think this is a good idea? And where do I start? The main.py file looks a little overwhelming for a C guy like me. But hey, I've pulled off the fuses and bootloader scripts in the past, so it should somehow be doable I think.

MCUdude commented 4 years ago

I think I have something that works. The only thing I can't figure out is where does upload_flags from platformio.ini pulled into main.py? I've looked everywhere, but I can't find any references to it. Still, the flags are added to every Avrdude command no matter what I do.

ivankravets commented 4 years ago

We even have internal TASK for this case :) It's historical issue with 2 targets. We totally agree that it should be ONE = upload. It's very often mistaken when developers do not know which target to use and in which case.

There is even strange docs for this https://docs.platformio.org/en/latest/platforms/atmelavr.html#upload-using-programmer

@MCUdude what are your ideas?

MCUdude commented 4 years ago

@MCUdude what are your ideas?

As I've explained in the post above, you can see how I can solve this for my Arduino cores. This is possible since we have the board_hardware.uart field, which reveals how the user wants to upload. I'm almost done with my crude implementation in main.py. The only thing I haven't figured out yet it how I can prevent the build_flags to always be added to the Avrdude command. Any hints on this exact issue? (And why does the upload flags has to be on separate lines? It makes it a pain to comment them out!)

But I do agree that we should strive to have one upload command that always works, and the preferences for the upload command should be defined in platformio.ini.

For other cores (i.e not Mega/Mini/Micro/Major/MightyCore) we need some sort of field that can be defined in order to tell the upload script that we don't want to use the "regular" bootloader upload, but rather the defined programmer. Any idea what this field should look like? Well, board_hardware.uart is already there.

MCUdude commented 4 years ago

@ivankravets @valeros I need your input on the following things:

How do I prevent the upload flags from being added to the Avrdude command? I've tried to comment out a bunch of lines in main.py, but I haven't succeeded.
I've explained how we can achieve one upload command with the Arduino cores I provide (by using board_upload.uart as an indicator). However, I will assume you're interested in implementing this for all AVR boards? We could focus on getting it to work with Mini/Mega/Mighty/Major/MicroCore first, and then figure out a clever way to make it work for all the others. I think we probably have to a new parameter that defines what to do if a programmer is defined (in upload_protocol).

MCUdude commented 4 years ago

@ivankravets I need some input from you in order to continue working on this. See the questions in my previous post

ivankravets commented 4 years ago

Sorry for the delay, we are crazy busy on the internal things. @valeros will back soon to this issue.

valeros commented 4 years ago

Hi @MCUdude ! Sorry for the late reply.

The program target is a vestige from the old times when the platformio.ini file was very limited in terms of functionality. Long story short, what if we ditch the program target if favor of the upload_command option? With a more comprehensive documentation on using it for uploading via an external programmer it could be a much flexible solution that gives users full control over upload flags.

MCUdude commented 4 years ago

@valeros no worries! I love PlatformIO so much I'll rather be patient rather than not helping with the development at all.

Long story short, what if we ditch the program target if favor of the upload_command option? With a more comprehensive documentation on using it for uploading via an external programmer it could be a much flexible solution that gives users full control over upload flags.

Well, we could ditch the upload using programmer command completely. But my idea was to tweak the pio run -t upload command a little so that this command could be used for every AVR upload procedure, not just for USB to serial adapters (that requires the -D flag). Using upload_command will work, but it will require way more configuration for the end-user than my idea would. Like I've mentioned in one of the earlier posts, I got a working example, but no matter what I can't get rid of the Avrdude flags.

valeros commented 4 years ago

Using upload_command will work, but it will require way more configuration for the end-user than my idea would. Like I've mentioned in one of the earlier posts, I got a working example, but no matter what I can't get rid of the Avrdude flags.

It seems that the approach you're proposing will work just fine only with your cores? What to do with all other boards that don't have board_hardware option by default?

MCUdude commented 4 years ago

It seems that the approach you're proposing will work just fine only with your cores?

Yes. Well, except that I haven't found a way to prevent the upload flags from being added to the Avrdude upload command if present in Platformio.ini.

What to do with all other boards that don't have board_hardware option by default?

Since this approach will make the program command " obsolete, we will need some kind of extra parameter in the manifest file to determine what to do if both upload_port and upload_protocol is defined in platformio.ini. IMO I think it would be best if we added board_hardware.uart to every AVR manifest. If the board comes default with a bootloader, we add uart0, uart1, uart2 or uart3. If it doesn't have a bootloader, we add no_bootloader. The whole idea is that this can be overridden in platformio.ini.

Take this configuration for instance:


[env:test_env]
platform = atmelavr
framework = arduino
board = uno
board_hardware.uart = no_bootloader

upload_protocol = usbtiny
upload_flags = -Pusb

This should automatically use the usbtiny programmer for uploading since no_bootloader is selected. change it to uart0, and it will use the autodetected serial port and the default upload protocol, stated in the manifest file (in this case, arduino).

valeros commented 4 years ago

If the board comes default with a bootloader, we add uart0, uart1, uart2 or uart3. If it doesn't have a bootloader, we add no_bootloader. The whole idea is that this can be overridden in platformio.ini.

I'm a bit reluctant to add that new hardware section to all manifests, mainly because we will need to check all boards in this repo to figure out what UART they use with bootloader. IMO, programming via an external programmer is a rare case, taking into account most of the boards in this platform already come with a bootloader.

MCUdude commented 4 years ago

I'm a bit reluctant to add that new hardware section to all manifests, mainly because we will need to check all boards in this repo to figure out what UART they use with bootloader. IMO, programming via an external programmer is a rare case, taking into account most of the boards in this platform already come with a bootloader.

For most development boards, that is true. However, for native AVR development (basically all my Arduino cores) uploading using a programmer is very much needed. In many professional applications, you don't want that bootloader installed. But could this be done so it only works with Mega/Mini/Micro/Mighty/MajorCore? Is so, that's fine with me. My cores cover pretty much all relevant ATmegas anyways.

valeros commented 4 years ago

But could this be done so it only works with Mega/Mini/Micro/Mighty/MajorCore?

Usually, we tend to avoid adding any core-specific features if there is an adequate solution based on existing functionality, just to reduce the maintenance burden. You mentioned that you have a preliminary PR that implements this feature, but the only problem is that you need to override the default upload_flags option?

Here is the code responsible for adding the upload flags to the final command. You can try to override the UPLOAD_FLAGS variable in the script, but IMO, it's not a good idea to mess with internal build system variables as it may somewhat confuse users when they specify upload_flags and they don't end up in the final upload command.

MCUdude commented 4 years ago

So what is the option of using upload_command? Would it be possible to specify a custom command, and it will be used if you either click the upload button is VSCode or by running pio run -t upload?

valeros commented 4 years ago

Would it be possible to specify a custom command, and it will be used if you either click the upload button is VSCode or by running pio run -t upload?

Yes, basically upload_command represents the precise command that will be invoked when you run the upload target in any form. Everything is specified solely in the platformio.ini file, for example:

[env:uno]
platform = atmelavr
framework = arduino
board = uno

; Upload using a programmer
upload_protocol = custom
upload_flags =
    -C
    $PROJECT_PACKAGES_DIR/tool-avrdude/avrdude.conf
    -p
    atmega328p
    -c
    atmelice_isp
upload_command = avrdude $UPLOAD_FLAGS -U flash:w:$SOURCE:i

MCUdude commented 4 years ago

Again my goal was to make setting fuses, load the bootloader, and upload procedure as sleek and streamlined as possible as long as platformio.ini is set up right.

if upload_command can override the command that's executed when pio run -t upload is run, the problem with uploading using a programmer is pretty much solved. Good!
One thing still remains: Being able to specify a programmer and flags for the programmer when setting fuses/bootloader without interfering with the standard USB to serial upload procedure.

Take this ini file for instance:

[env:MiniCore]
platform = atmelavr
framework = arduino

board = ATmega328P

# Serial upload
board_upload.speed = 115200
upload_port = /dev/cu.usbserial-1420

# programmer and flags for fuses/bootloader
upload_protocol = usbtiny
upload_flags =
  -Pusb
  -B32

I would like to be able to upload using a USB to serial adapter and set the fuses / load bootloader without having to comment out upload_flags since these are always added to the upload command. upload_flags should ideally only be added when upload_protocol is used, which is when the user sets fuses/bootloader. It's so much nicer if a project can have a platformio.ini file that you don't need to modify just to set fuses/bootloader, and undo those changes again because you want to upload your code.

It's only because upload_flags is added to the upload command, even though there's no need for it unless you're using a programmer (for fuses/bootloader). I hope you understand what I'm trying to express 😅

All that's needed is to add a trigger somewhere. Add upload_flags to the Avrdude command if the user runs pio run -t fuses or pio run -t bootloader. Do not add upload_flags to the Avrdude command if the user runs pio run -t upload. I don't see why custom upload flags would ever be necessary when you're uploading using a bootloader anyways. If the user for some reason still need custom flags, he can just add them the the upload_port field after the specified serial port.

valeros commented 4 years ago

All that's needed is to add a trigger somewhere. Add upload_flags to the Avrdude command if the user runs pio run -t fuses or pio run -t bootloader. Do not add upload_flags to the Avrdude command if the user runs pio run -t upload.

IMO, that sounds a bit misleading to me. upload_flags was exactly created to append flags to the upload command. The documentation states

Extra flags for uploader. Will be added to the end of uploader command.

We cannot play with its behavior across platforms.

MCUdude commented 4 years ago

We cannot play with its behavior across platforms.

OK, I understand that. So could we have two new parameters specifically for fuses and bootloader? For instance programmer_flags and programmer_protocol?

ivankravets commented 4 years ago

Sorry for the interrupt, I can't understand why do we need again multiple options. So, people hate tons of these options, and we plan to back them again?

The solution with upload_command is not bad. So, we will give developers the freedom and they can configure as they need. We will document use cases at https://docs.platformio.org/en/latest/platforms/atmelavr.html#upload-using-programmer ( @valeros , please update docs using new upload_command )
If someone needs different upload_commands - there are working environments https://docs.platformio.org/en/latest/projectconf/section_env.html So, you declare the base environment and then override with your custom, for example:

[env]
platform = atmelavr
framework = arduino

[env:uno]
board = uno

[env:ATmega328P]
board = ATmega328P
upload_command =
   custom
   args

[env:ATmega328P_set_fuses]
board = ATmega328P
upload_command =
   custom
   args

MCUdude commented 4 years ago

@ivankravets you guys know the PlatformIO build system the best. Using different environments is something I haven't even thought of! However, will it a "custom" environment work for the fuses and bootloader command? Will the fuses and bootloader hex be added to upload_command?

If yes, all this needs to be properly documented with multiple examples. But it is fine for me!

valeros commented 4 years ago

At the moment the best we can do is the following:

In the latest changes I tried as much as possible to keep the current workflow for fuses and bootloader targets, but also tried to open new ways for more flexible customization. Now, both fuses and bootloader commands are split into separate parts that can be easily overridden in an extra script (partially or the entire command). For example the fuses target executes the following command:

$FUSESUPLOADER $FUSESUPLOADERFLAGS $UPLOAD_FLAGS $FUSESFLAGS

FUSESUPLOADER - the tool used for setting fuses
FUSESUPLOADERFLAGS - general command line options that control uploader's behavior (e.g. -D, -V, -P COM1)
FUSESFLAGS - a list of flags specific to fuses settings (generated dynamically or set in the manifest file). For example -Ulock:w:0x2F:m, -Uefuse:w:0xCB:m, -Ulfuse:w:0xFF:m)

Each part can be overridden in an extra script, for example:

Import("env")

env.Replace(
    FUSESUPLOADERFLAGS=[
        "-C", "$PROJECT_PACKAGES_DIR/tool-avrdude/avrdude.conf",
        "-p", "$BOARD_MCU",
        "-c", "atmelice_isp",
        "-e", "-v"
    ],
    # Set only lock bits
    SETFUSESCMD="avrdude $FUSESUPLOADERFLAGS -Ulock:w:0x0F:m",
)

or replace only the fuses:

Import("env")

env.Replace(
    FUSESFLAGS=[
        "-Uhfuse:w:0xAA:m",
        "-Uefuse:w:0xBB:m",
        "-Ulfuse:w:0xCC:m",
        "-Ulock:w:0xDD:m"
    ]
)

Now combining this approach with the previous idea with separate environment for setting fuses we can put together something like this:

[env]
platform = atmelavr
framework = arduino
[env:ATmega328P]
board = ATmega328P

[env:ATmega328P_set_fuses]
; Use default or specify custom settings in `override_fuses_cmd.py`
extra_scripts = 
   override_fuses_cmd.py

[env:ATmega328P_upload_using_BP_programmer]
board = ATmega328P
upload_protocol = custom
upload_port = COM5
upload_speed = 115200
upload_flags =
    -C
    $PROJECT_PACKAGES_DIR/tool-avrdude/avrdude.conf
    -p
    $BOARD_MCU
    -P
    $UPLOAD_PORT
    -b
    $UPLOAD_SPEED
    -c
    buspirate
upload_command = avrdude $UPLOAD_FLAGS -U flash:w:$SOURCE:i

This approach can also be used with atmelmegaavr platform. @MCUdude What do you think?

MCUdude commented 4 years ago

Now, both fuses and bootloader commands are split into separate parts that can be easily overridden in an extra script (partially or the entire command)

OK, so this means that pio run -t fuses and pio run -t bootloader now has nothing to do with each other. The fuses script isn't automatically run when bootloader runs? If so, that's a good thing. Can the fuses still be manually overridden in platformio.ini without having to use an extra_script to do so? I know many users find this convenient.

I'll have to do more testing later to see if everything works as expected, and at least makes sense.

valeros commented 4 years ago

OK, so this means that pio run -t fuses and pio run -t bootloader now has nothing to do with each other. The fuses script isn't automatically run when bootloader runs? If so, that's a good thing.

The changes were mostly made to increase flexibility so users can override any part of the final command. The behavior is still the same: the bootloader target invokes fuses command.

Can the fuses still be manually overridden in platformio.ini without having to use an extra_script to do so? I know many users find this convenient.

Yes, it's still possible to override fuses in platformio.ini.

MCUdude commented 4 years ago

OK, so I've made myself a platformio.ini file that would let me upload using a bootloader, upload using a programmer and set fuses/burn bootloader:

[env]
platform = https://github.com/platformio/platform-atmelavr.git
framework = arduino
board = ATmega1284P

[env:ATmega1284P_upload]
board_upload.speed = 115200
upload_port = /dev/cu.usbserial-1420

[env:ATmega1284P_fuses]
board_bootloader.speed = 115200
upload_protocol = usbasp
upload_flags =
  -PUSB

[env:ATmega1284P_upload_using_programmer]
upload_protocol = custom
upload_flags =
    -C
    $PROJECT_PACKAGES_DIR/tool-avrdude/avrdude.conf
    -p
    $BOARD_MCU
    -P
    USB
    -c
    usbasp
upload_command = avrdude $UPLOAD_FLAGS -U flash:w:$SOURCE:i

In VSCode it if fairy easy to upload using bootloader or upload using the programmer. All you have to do is to select the correct environment on the blue line and hut the upload button. Setting fuses/bootloader is a bit more complicated since the command now also has to specify the environment: pio run -t fuses -e ATmega1284P_fuses.

Would it be possible to have an environment like this:

[env:ATmega1284P_fuses]
board_bootloader.speed = 115200
upload_protocol = usbasp
upload_flags =
  -PUSB
extra_scripts = 
  fuses.py

So you theoretically could just select the ATmega1284P_fuses environment in VSCode, hit upload, and the fuses would be set without trying to upload? And if you use bootloader.py instead, both the fuses and bootloader script will be run. If so, that would be really neat! Especially for those who don't usually work with the command line, or fully understands the whole environment concept. Does this make sense to you?

valeros commented 4 years ago

So you theoretically could just select the ATmega1284P_fuses environment in VSCode, hit upload, and the fuses would be set without trying to upload?

Unfortunately, that won't work. Users still need to run specific targets (fuses or bootloader) so we can calculate values for them. An extra script is handly when default command for fuses or bootloader targets is not suitable for any reason.

MCUdude commented 4 years ago

Unfortunately, that won't work. Users still need to run specific targets (fuses or bootloader) target so we can calculate values for them. An extra script is handly when default command for fuses or bootloader targets is not suitable for any reason.

OK, so there's no simple way to make this work either, that doesn't break with any major principles I might not be aware of? As you can imagine, this would have been a very elegant solution, especially for those who use a graphical environment such as VSCode. Almost as easy as in Arduino IDE!

MCUdude commented 4 years ago

OK, so there's no simple way to make this work either, that doesn't break with any major principles I might not be aware of?

Yes there is!

[env:ATmega1284P_fuses]
board_bootloader.speed = 115200
upload_protocol = usbasp
upload_flags =
  -PUSB
upload_command = pio run -e ATmega1284P_fuses -t fuses

It is pretty much a hackjob, but it does kinda work! The only "issue" here is that the code has to compile without error for the custom upload command to run.

EDIT: BTW is there a way to get the current environment name, so an upload_command like this would be valid?

[env:ATmega1284P_fuses]
board_bootloader.speed = 115200
upload_protocol = usbasp
upload_flags =
  -PUSB
upload_command = pio run -e ${env.this} -t fuses

valeros commented 4 years ago

It is pretty much a hackjob, but it does kinda work!

:+1: It seems hacky to me as well, but great that it works for you .

BTW is there a way to get the current environment name, so an upload_command like this would be valid?

Unfortunately there is no such way.

Just to wrap up. At the moment the current implementation it's the best we can do, so I propose to reuse the same workflow with the atmelmegaavr platform.

MCUdude commented 4 years ago

BTW is there a way to get the current environment name, so an upload_command like this would be valid? Unfortunately there is no such way.

Would a variable like $ENVIROMENT that's local to each environment even be possible with a simple PR, or is this a fundamental thing that pretty much not possible? It would have been great if it existed. If yes, would the change apply to the atmelavr main.py, or some other

Just to wrap up. At the moment the current implementation it's the best we can do, so I propose to reuse the same workflow with the atmelmegaavr platform.

Great, this works as long as it is documented properly because it needs a little more setup. But that's fine 🙂 And as far as I understand, this doesn't break any backward compatibility either? It would be nice to be able to do the same thing for atmelmegaavr too.

Off topic: About the atmelmegaavr MegaCoreX implementation. The thread has become a bit noisy I think, but I hope we can stick to one manifest file for each chip (atmega4809.json, atmega4808.json, etc.) and rather use board_build.variant to specify the pinout in use, just what we do on my other cores. It would also be great if the wait_for_upload_port was automatically determined when the programmer is specified, but that's a simple fix that can be applied in main.py.

Almost on topic: I installed PlatformIO on a new computer yesterday, and I noticed that it pulled the latest version of MiniCore (v2.0.7). Have you manually updated what version PlatformIO should pull, or is it handled automatically?

On topic: When will the next version of the atmelavr package be released?

valeros commented 4 years ago

Would a variable like $ENVIROMENT that's local to each environment even be possible with a simple PR, or is this a fundamental thing that pretty much not possible?

I suppose it's possible and probably should be implemented in platformio-core.

And as far as I understand, this doesn't break any backward compatibility either? It would be nice to be able to do the same thing for atmelmegaavr too.

I should be backward compatible, but it seems better to publish a new release of the platform with incremented major version, just in case.

About the atmelmegaavr MegaCoreX implementation. The thread has become a bit noisy I think, but I hope we can stick to one manifest file for each chip (atmega4809.json, atmega4808.json, etc.) and rather use board_build.variant to specify the pinout in use, just what we do on my other cores.

I agree, there is no sense to multiply manifests that only differ by the variant.

It would also be great if the wait_for_upload_port was automatically determined when the programmer is specified, but that's a simple fix that can be applied in main.py.

Will do.

I installed PlatformIO on a new computer yesterday, and I noticed that it pulled the latest version of MiniCore (v2.0.7). Have you manually updated what version PlatformIO should pull, or is it handled automatically?

Is there a chance that you installed the upstream version? I've updated your packages recently in the dev branch, but since a new release of the platform is still on the way, they shouldn't be publicly available.

When will the next version of the atmelavr package be released?

I hope it will be released today :smile:

MCUdude commented 4 years ago

Would a variable like $ENVIROMENT that's local to each environment even be possible with a simple PR, or is this a fundamental thing that pretty much not possible?

I suppose it's possible and probably should be implemented in platformio-core.

Awesome. I could make a new platformIO template that utilizes this; One environment that the user can use for "regular" uploads with a bootloader, another environment for uploading using a programmer, and a third environment for setting fuses/bootloader. I'm sure users will love how convenient this is!

It would also be great if the wait_for_upload_port was automatically determined when the programmer is specified, but that's a simple fix that can be applied in main.py.

Will do.

Thanks! As I've mentioned in another thread earlier, jtag2updi is the only UPDI programmer that requires an upload port.

EDIT: Oh, and "regular" uploading using a bootloader (Optiboot). /EDIT

I installed PlatformIO on a new computer yesterday, and I noticed that it pulled the latest version of MiniCore (v2.0.7). Have you manually updated what version PlatformIO should pull, or is it handled automatically?

Is there a chance that you installed the upstream version? I've updated your packages recently in the dev branch, but since a new release of the platform is still on the way, they shouldn't be publicly available.

The computer doesn't even have git installed, so I only used platform = atmelavr. I also saw there was an update available for the atmelavr package under platformIO home. I already had 2.2.0 installed, but it still updated a bunch of my cores to the latest version. Not sure what causes this when you say they shouldn't be publically available yet.

MCUdude commented 4 years ago

Totally forgot to close this!

platformio / platform-atmelavr

Improve upload/program logic #209