Control Pounder through MQTT - finished

[Spaqin]

Since @occheung is busy with other things in life, I was tasked with finishing his PR (see #610 for more details). It looked complete enough for me; I only applied comments and fixed minor issues.

Tested with a Pounder 1.1.

[jordens]

If this doesn't differ from #610 you'll still need to address the issues raised there.
If this does differ from #610 you'll need to figure out a way to make the changes since the last review visible and reviewable and you still need to explain how you addressed the issues raised there.
Do you want to close the old PR?
In general you have a better and easier path forward if you'd split this up into multiple PRs.

[jordens]

?
Please do read your own diff.

[Spaqin]

The commits I've made on top of the old PR are rather easily visible here in the github UI, I think. And thankfully I didn't have much to address myself either; rather just follow directions of existing comments. Unfortunately, there's no magical way to move the comments over to another PR... or maybe if that's not enough I can wait for occheung for his additional helpful input, and maybe merge the changes into his old branch and continue from there.

[PhBrb]

I am having problems to flash the stabilizer, which might be related to this PR.
With probe-run I can run the firmware (cargo run --release --bin dual-iir --features pounder_v1_0) and also control the pounder through MQTT. But after a powercycle of the stabilizer, it panics (led 1 and 3 are on). Because the powercycle stopped the debugging session I can not see what causes the panic. Other methods of flashing also do not work (DFU upload or cargo build ... & cargo objcopy + moving to flash storage), after a powercycle the stabilizer panics.
Flashing the main branch at v0.7.0 without the pounder attached works.
This was done on a stabilizer v1.1 & pounder v1.0.
Triggering this bug is weird.
I use cargo run..., power off the stabilizer, disconnect the ST-Link, power on the stabilizer and it panics.
I use cargo run..., power off the stabilizer, keep ST-Link connected, power on the stabilizer and it works as expected.
I flash over flash storage, power off the stabilizer, disconnect ST-Link, power on the stabilizer and it panics.
I flash over flash storage, power off the stabilizer, keep ST-Link connected, power on the stabilizer and it panics.

As suggested by @jordens if I use cargo run --release --bin dual-iir --features pounder_v1_0 -- --no-flash after it paniced, I get

     Running `probe-run --chip STM32H743ZITx --speed 30000 target\thumbv7em-none-eabihf\release\dual-iir --no-flash`
(HOST) INFO  skipped flashing
────────────────────────────────────────────────────────────────────────────────
INFO - Starting
INFO - EUI48: 04-91-62-d2-60-2f
INFO - Found Pounder
panicked at 'called `Result::unwrap()` on an `Err` value: Check', src\hardware\setup.rs:879:30
────────────────────────────────────────────────────────────────────────────────
(HOST) INFO  device halted without error```

[ryan-summers]

The commits I've made on top of the old PR are rather easily visible here in the github UI, I think. And thankfully I didn't have much to address myself either; rather just follow directions of existing comments. Unfortunately, there's no magical way to move the comments over to another PR... or maybe if that's not enough I can wait for occheung for his additional helpful input, and maybe merge the changes into his old branch and continue from there.

In my opinion, it would make the most sense to commit directly to the previous branch so that we can keep all the review comments that still exist there. There's a lot of stuff that I have no idea if it's been fixed or not.

[ryan-summers]

@PhBrb Please open an issue for us to help you - commenting on this PR isn't a great way to get visibility on your issue, and I don't see how it's related to this PR, outside of pounder being present?

The Check error results when Stabilizer cannot talk to the AD9959 chip properly, so I suspect there's something else going on. In general, I wouldn't recommend using probe-run for permanent flashing (it's a debugging tool). Use cargo flash instead.

[PhBrb]

@PhBrb Please open an issue for us to help you - commenting on this PR isn't a great way to get visibility on your issue, and I don't see how it's related to this PR, outside of pounder being present?

The Check error results when Stabilizer cannot talk to the AD9959 chip properly, so I suspect there's something else going on. In general, I wouldn't recommend using probe-run for permanent flashing (it's a debugging tool). Use cargo flash instead.

@ryan-summers I do think the issue is related to this PR. When I played around with the v0.7 release and got the Pounder running with it, I did not run into that issue. I was able to permanently flash with cargo run and cargo flash and control the Pounder through MQTT (uploaded here https://github.com/PhBrb/Stabilizer-Pounder/commit/6a4a6f8443bd73cd4a15d5633f10f55f182c2320 ). So it's not the hardware or the v0.7 that is causing it, but the changes made in this PR. Or I accidentally fixed an existing bug while playing around, but I doubt that.

[Spaqin]

@PhBrb Please open an issue for us to help you - commenting on this PR isn't a great way to get visibility on your issue, and I don't see how it's related to this PR, outside of pounder being present?
The Check error results when Stabilizer cannot talk to the AD9959 chip properly, so I suspect there's something else going on. In general, I wouldn't recommend using probe-run for permanent flashing (it's a debugging tool). Use cargo flash instead.

@ryan-summers I do think the issue is related to this PR. When I played around with the v0.7 release and got the Pounder running with it, I did not run into that issue. I was able to permanently flash with cargo run and cargo flash and control the Pounder through MQTT (uploaded here PhBrb/Stabilizer-Pounder@6a4a6f8 ). So it's not the hardware or the v0.7 that is causing it, but the changes made in this PR. Or I accidentally fixed an existing bug while playing around, but I doubt that.

Hi @PhBrb - I thought I replied to you through email back in April (...GitHub email states that you can just reply through email, after all) - that didn't work for whatever reason. No big deal.

Let me share the contents of these emails as they're still relevant:

---

Thank you for putting light onto the issue. Since I tested the Pounder right after flashing (as still working on it) and CI tests run also right after flashing, it's not trivial to catch in usual development environment.

And I agree, it's weird. I could reproduce the issue as well, but with a caveat - in my case it does not panic if it's power cycled with the ST-Link connected. If I disconnect it, it panics. The cause is the same as well.

And yes I can also confirm it seems to be fine with 0.7.0 release.

I looked through the changes of that PR and there was nothing really suspicious... However, (and thankfully for me!) it also happens if you try to use the master, I mean, main branch. I'll try to find the commit that introduced this bug; could be a random dependency update that did that.

The commit that introduced the issue was this: 2f4e714 - bump stm32h7xx-hal from 0.13.0 to 0.13.1.

But of course I had to go deeper into it and see what changed in the HAL to make that problem come...

This was it:

stm32-rs/stm32h7xx-hal@dd73c1e

So if you clone the HAL, check out to 0.14.0, and comment out these two lines (638, 665; marked as beginning and end on GitHub): https://github.com/stm32-rs/stm32h7xx-hal/blob/b4fb6de6fb34ea77269a951ce366601e4bab3915/src/i2c.rs#L638,L665

then change the path to the library in Cargo.toml dependencies, it works again.

Why adding a busy_wait! in i2c caused an issue with a QSPI controller only after reboot only if ST-Link is not connected, that's beyond me; I'm also not sure how necessary that is.

[ryan-summers]

@PhBrb / @Spaqin I wanted to let you both know about #746 - it appears there's an issue with Stabilizer+Pounder bootup that's largely independent of this PR, and is caused by the power-up sequence of the devices on Pounder

[jordens]

closing as stale. feel free to resurrect if there is momentum.