Added support for atmega1280 and bigavr6

[peacememories]

do not merge this yet

This addition does not work yet and crashes the compiler.

Both examples work now. The port definitions are not at all complete though.]

Also I don't have any idea yet on how to add stuff like a display that can occupy exactly one set of pins (because it is hardwired there) and is easy to create.

[Rahix]

I think the pins are not correct, for example the datasheet lists PL0, PL1, PL2, PL3, PL4, PL5, PL6, PL7, while you only define PL0, PL1, PL4, PL5, PL6, PL7. Similar for the other ports ...

You could also add the other UART peripherals without much more work as well, if you want

[peacememories]

Yeah, I purposely only added what was needed for the examples to run for now. I can add the rest in a few hours if you’d like

[peacememories]

Oh, no, the pl* missing might just have been me overlooking something when copying

[peacememories]

Pins fixed, USARTs added to the hal. in the bsp, should i replace Serial with SerialN for each USART?

[Rahix]

I don't know much about BIGAVR6, so I can only guess what is right ... I reexported USART0 in arduino-leonardo, because it is the primary serial device and I wanted it to be easily accessible. If there is one serial that is primarily used on BIGAVR6, I'd suggest just reexporting that.

[Rahix]

For the pins: I don't think it makes sense to create the Pins struct at all if the names in there are just the same as for the actual chip's pins. Instead, the following "user" code would do the same:

let dp = bigavr6::Peripherals::take().unwrap();
let mut porta = dp.PORTA.split();
let led = porta.pa0.into_output(&mut porta.ddr);

The only reason for the Pins struct is to rename the pins to more obvious names, specific to a hardware. For example on Arduino Leonardo, there are names printed on the PCB, and I wanted to be able to just use them without having to cross-reference the circuit diagram each time.

Now, I don't know how BIGAVR6 looks, so you'll have to decide if there is some sensible naming scheme. If not, then just remove the Pins struct definition entirely and let users just reference the ports directly.

[peacememories]

Okay, I'll remove the pin struct, and I'll change the reexport of the USART. All of the USARTs are pinned out on the BIGAVR6, but iirc there is a "Serial1" and "Serial2" with actual Serial Port headers, so exporting those with the same name as on the board might make sense. Thanks for your feedback :)

[Rahix]

By the way, you can remove Xargo.toml now that the new compiler has been released. Also, please make the target file (avr-atmega1280.json) a symlink to the actual target file located in the chips/atmega1280-hal/ directory. You can take a look at the existing crates to see how I did it.

[peacememories]

By the way, you can remove Xargo.toml now that the new compiler has been released.

Say what now?! how did I miss this?

Will do!

btw: the board itself labels two serial ports RS-232 A and B respectively. What do you think would be more understandable? SerialA or RS232A?

[Rahix]

Hmm, I'd argue RS232 is just the name of the connector while the protocol is 'Serial'. From the software side we only care about the protocol, so I'd call it SerialA and SerialB.

[peacememories]

Removed pins, correctly specified serial interface (i actually remembered that rs232a and b are actually on the same usart, with dip-switches to choose between them 🙄 )
Also moved the target json and Xargo.toml

[peacememories]

Hmm, when i try to implement the panic example, I get the following output:

Hello from BIGAVR6!
Firmware panic!
  At boards/bigavr6/examples/bigavr6-panic.rs:Firmware panic!
  At :9:4

9:4 is the line where serial is initialized in the panic handler, which seems like a bogus value, and apparently the panic handler panics when trying to get/format the line number?

[Rahix]

Hmm, this is really weird ... Technically, the USARTx struct should be zero-sized so there is no code actually generated for that line ...

Can you try replacing the hackish initialization with a proper one like

    let dp = unsafe { bigavr6::Peripherals::steal() };

    let mut porte = dp.PORTE.split();
    let mut serial = bigavr6::Serial::new(
        dp.USART0,
        porte.pe0,
        porte.pe1.into_output(&mut porte.ddr),
        57600,
    );

please?

[peacememories]

Apparently the problem is somewhere in the formatting. If I don't use all of the elements (for example, only file:line, or file:column), or if i substitute one of them with a constant, it works, but as soon as I use all of them it crashes.

I tried the different initialization logic, but it still crashes

[peacememories]

Side note: this is done using an atmega1280 simulator, since I don't have a board here right now. So there could be an issue with the simulator, but I don't think it's that likely.
A colleague suggested that maybe it has something to do with the uart buffers running over. I'm not sure if the serial implementation checks if it can send.

[Rahix]

Huh, ok ... Try splitting the uwriteln!() call into multiple and see if that works ...

I'm not sure if the serial implementation checks if it can send.

I am pretty sure it does. At the beginning of write, I first call flush to ensure previous data has been sent. (In fact, there is no buffer, there is only ever one byte)

[peacememories]

I've done this now:

ufmt::uwrite!(&mut serial, "  At {}:",
    loc.file()
).unwrap();
ufmt::uwrite!(&mut serial, "{}:",
    loc.line()
).unwrap();
ufmt::uwriteln!(&mut serial, "{}",
    loc.column()
).unwrap();

This crashes. A bit of testing showed: whether the file is printed doesn't matter, it only crashes if i use line and column O.o

[Rahix]

Right, this might make sense: line and column are both u32 (docs). Maybe the formatting code for u32 is broken in some way? Although I had it work on ATmega32U4, so I would be kind of surprised if it is ...

[Rahix]

Try formatting a u32 in main

[peacememories]

Formatting line/column and a static u32 seems to work though

[peacememories]

I have to go now, I'll probably be back online in an hour or so. Thanks so much for your help! :)

[peacememories]

Oookay, I changed the hello world line to

ufmt::uwriteln!(&mut serial, "Hello from BIGAVR6!{}", 32u32).unwrap();

aaand it crashes:

I think that was just a fluke. It still crashes afterwards

[peacememories]

Okay, I'm getting closer: It crashes anytime I have two u32 formats in my code. But it crashes on the first one. So that might be a problem with the formatter on one hand, but maybe also with the macro itself.

[Rahix]

Hmm ... Can you try using the ufmt::Formatter API directly, without the uwriteln! macro? Maybe that might help in pinpointing the issue a little further ...

[Rahix]

I might have some time this weekend to spin up the avr simulator myself and try to reproduce the issue as well ...

[peacememories]

Using only fmt to send the numbers results in the same crash. If i only use it once it works, if i use it twice (once in main, once in the panic handler after printing the file name) it crashes at the first one.

[Rahix]

I am able to reproduce your issue ... It is very strange and to be honest feels a lot like a compiler bug. For example, compiling in debug works totally fine. Under some circumstances, in release it works as well, but with others it does not ...

[peacememories]

Does the generated assembly show any hints of why it goes bad sometimes?
Should we submit a bug report to the compiler repo?

[Rahix]

I really don't know ... From what I can see, it only happens if a panic handler is involved, which is really weird. It works perfectly fine if its just in main, but as soon as I add the panic!() statement, it already panics in main ...

The observation that it needs two formattings to happen is also interesting. I'd guess this stems from a single formatting being inlined while for two, the compiler creates a separate function.

We should definitely submit this upstream, but only after reducing the failing code to a minimal example. Would you be interested in doing that?

[Rahix]

Wow ... After retrying my Arduino Leonardo version, it now stopped working as well (on real hardware as well) ... I have no idea what is different now

[Rahix]

Huh ... So it works with optlevel = 3, but not with optlevel = "s" ...

[Rahix]

Unfortunately I won't have much time next week. If you want to continue tracking this down, I would suggest you try reducing the example so you end up with a minimal version (one .rs file, with no dependencies or just ufmt if absolutely necessary). Replace all the avr-hal stuff with direct writes to the respective registers, like

core::ptr::volatile_write(0xF0 as *mut u8, 0xBA);

(Only if you really feel like torturing yourself like that ...)

[Rahix]

I could not leave it alone ... I took a look with the debugger and found that the uxx::usize formatter for some reason does not have a ret instruction at the end which means it just falls through to the panic again. It does however have a return instruction for the case that the number is smaller than 10, which is why the second panic did not drop into an endless loop. You can try it out, with numbers smaller than 10, it works, but numbers bigger than 10 do not. Code:

#[panic_handler]
fn panic(info: &core::panic::PanicInfo) -> ! {
    let mut serial: bigavr6::Serial<bigavr6::hal::port::mode::Floating> = unsafe {
        core::mem::uninitialized()
    };

    // Using 0x9 instead of 0xA works ...
    unsafe { core::ptr::write_volatile(0x3E as *mut u8, 0xA); }

    ufmt::uwriteln!(&mut serial, "Firmware panic!\r").unwrap();

    if let Some(loc) = info.location() {
        ufmt::uwriteln!(
            &mut serial,
            "  At {}:{}:{}\r",
            loc.file(),
            unsafe { core::ptr::read_volatile(0x3E as *mut u8) } as u8,
            unsafe { core::ptr::read_volatile(0x3E as *mut u8) } as u8,
        );
    }

    loop {}
}

[Rahix]

Alright, there it is: ufmt uses a macro called assume_unreachable!() for their panic-less implementation. This macro either expands to the unreachable!() panic call in debug mode (hence it worked there) or the core::hint::unreachable_unchecked() hint in release mode. This hint is probably the reason no ret was generated (which is still a compiler bug).

We only see this bug in this form when using a panic because the uxx::usize impl was put right in front of the rust_begin_unwind impl.

[Rahix]

I have reported the issue upstream: avr-rust/rust-legacy-fork#148

[peacememories]

Thanks a lot. I’m currently not in a shape to sit in front of a computer, so I’m thankful you tracked down the bug :) as soon as I’m better I’ll continue fleshing out the pr :)

[Rahix]

Alright, take your time to get better, I wish you the best! I will continue working on getting this fixed upstream. Don't consider the bug blocking for your PR.

[Rahix]

I have pushed a workaround for the bug you found. I also took another look at this PR. From my perspective, it looks good to merge. Is there anything you still want to work on before I do so?

[Rahix]

@peacememories, can we merge this?

[peacememories]

@Rahix sorry for the late reply. Yes it can technically be merged. It's not finished yet, but what is there should be working. Unfortunately I can't work on the PR right now since my avr-rustc is broken^^'

[Rahix]

Completely forgot to mention it here: I went ahead and manually merged your work in commit a8b2a8a. I hope that is ok with you ... So even though this PR will be marked as closed, your work is actually now integrated into master!