Rahix / avr-hal

embedded-hal abstractions for AVR microcontrollers
Apache License 2.0
1.32k stars 223 forks source link

BoardAvrdudeOptions::baudrate not being overwritten by RavedudeGeneralConfig::serial_baudrate from Ravedude.toml #599

Open fernando-isensee opened 4 days ago

fernando-isensee commented 4 days ago

Hello, I would like to first apologize in advance if I am not raising this issue in a proper way, or if this is a nonissue and I am using the code incorrectly, or if it has been already fixed and will be released soon.

The Issue

I wanted to make a custom board based on atmega1284p to use with the libraries, but I was not able to program the device. The issue was that the BoardAvrdudeOptions::baudrate variable was not being overwritten by the serial_baudrate [generic] settings I placed in Ravedude.toml.

1) Add the following code to Boards.toml, with unspecified baudrate

[atmega1284p]
    name = "Atmega1284p chipset board"

    [atmega1284p.reset]
    automatic = true

    [atmega1284p.avrdude]
    programmer = "arduino"
    partno = "atmega1284p"
    baudrate = -1
    do-chip-erase = true

    [atmega1284p.usb-info]
    # No IDs here because the Nano 168 uses a generic USB-Serial chip.
    error = "Not able to guess port"

2) Setup the runner in my project's config.rs file:

[target.'cfg(target_arch = "avr")']
runner = "ravedude"

3) Setup the Ravedude.toml file with

[general]
board = "atmega1284p"
serial-baudrate = 38400
port = "COM8"
open-console = false

4) Result without the fix:

>cargo run
    Finished dev [optimized + debuginfo] target(s) in 0.03s
     Running `ravedude target\avr-atmega1284p\debug\mcu17-test.elf`
       Board Atmega1284p chipset board
 Programming target\avr-atmega1284p\debug\mcu17-test.elf => COM8
avrdude warning: attempt 1 of 10: not in sync: resp=0x00
avrdude warning: attempt 2 of 10: not in sync: resp=0x00

This happened because baudrate was being read from the file Boards.toml, which is -1, so it ended up being converted to Some(Some(None)).

My proposed fix

1) I changed main.rs to from line 169 to

  // Make board_avrdude_options mutable so that we can edit it's baudrate
  let mut board_avrdude_options = board
      .avrdude
      .take()
      .ok_or_else(|| anyhow::anyhow!("board has no avrdude options"))?;

  // Change board's baudrate if the serial-baudrate has been configured in Ravedude.toml
  if !ravedude_config.general_options.serial_baudrate.is_none() {
      board_avrdude_options.baudrate = Some(ravedude_config.general_options.serial_baudrate);
  }

2) Result with the fix:

>cargo run
    Finished dev [optimized + debuginfo] target(s) in 0.03s
     Running `ravedude target\avr-atmega1284p\debug\mcu17-test.elf`
       Board Atmega1284p chipset board
 Programming target\avr-atmega1284p\debug\mcu17-test.elf => COM8
avrdude: AVR device initialized and ready to accept instructions
avrdude: device signature = 0x1e9705 (probably m1284p)
avrdude: erasing chip

avrdude: processing -U flash:w:target\avr-atmega1284p\debug\mcu17-test.elf:e
avrdude: reading input file target\avr-atmega1284p\debug\mcu17-test.elf for flash
         with 546 bytes in 1 section within [0, 0x221]
         using 3 pages and 222 pad bytes
avrdude: writing 546 bytes flash ...
Writing | ################################################## | 100% 0.38 s
avrdude: 546 bytes of flash written
avrdude: verifying flash memory against target\avr-atmega1284p\debug\mcu17-test.elf
Reading | ################################################## | 100% 0.29 s
avrdude: 546 bytes of flash verified

avrdude done.  Thank you.
Rahix commented 3 days ago

Hi, thanks a lot for the detailed report :) And thanks for testing the new ravedude config file code!

It would be best if you send your proposed fix as a pull-request, that makes it easier to discuss. In general, if you have code change suggestions, always open a pull-request for them.

Without having all the details in my head right now, some thoughts:

Cc: @Creative0708