PX4 / PX4-Bootloader

PX4 Bootloader for PX4FMU, PX4IO and PX4FLOW
Other
266 stars 547 forks source link

Fixes erroneous USB VBUS Sense #96

Closed ksschwabe closed 3 years ago

ksschwabe commented 6 years ago

The initilaisation, reading and deinitiliasation of PA9 is preformed all in board_test_usb_vbus().

This also solves the problem, where PA9 was floating and could erroneously have been read as being HIGH, and hence triggered the board to stay in the bootloader until timeout, instead of booting immediately.

Fixes issue #82.

davids5 commented 6 years ago

How about an |= for all sets except the first?

On January 23, 2018 5:19:44 AM HST, ksschwabe notifications@github.com wrote:

ksschwabe commented on this pull request.

@@ -764,14 +785,13 @@ main(void)

if defined(BOARD_USB_VBUS_SENSE_DISABLED)

try_boot = false;

else

-

  • if (gpio_get(GPIOA, GPIO9) != 0) {
  • if (board_test_usb_vbus()) {

We will have to be careful of doing try_boot = board_test_usb_vbus();, since try_boot is simply a flag that can be set to false by a number of different tests (receiving a UART break, if boot pin is strapped, if USB present). If we do the check try_boot = board_test_usb_vbus(), it would overwrite the previous check, which we don't want. We simply want a Or, check where if any of the checks returns true, then don't boot.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/PX4/Bootloader/pull/96#discussion_r163275042

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

ksschwabe commented 6 years ago

I have now put the #if defined(BOARD_USB_VBUS_SENSE_DISABLED) into the board_test_usb_vbus() function.

I have restructured all of those if (some_test()) {try_boot = false;} tests to be bitwise-and assignments, i.e. try_boot &= !some_test();. That has cleaned up the main function a bit more. I don't know if you find the &= !<some_test()> syntax confusing in comparison to the more usual |= some_test(). If you would rather have them as |= assignments, then we will have to change try_boot to stay_in_bootloader. What do you think?

ksschwabe commented 6 years ago

Reverted to using ifs.

ksschwabe commented 6 years ago

@dagar : Are you happy with this fix?

dagar commented 6 years ago

I'm not really familiar with this code, but it looks good to me. One minor nitpick is the formatting is a bit of a mess. See the github diff.

ksschwabe commented 6 years ago

@dagar : That should have fixed the formatting problem. The changes should now be consistent with the rest of the file.

ksschwabe commented 6 years ago

Are we happy with this PR? Do I need to make some additional changes?

ksschwabe commented 6 years ago

I've rebased this on master.

LorenzMeier commented 3 years ago

@davids5 What is the state here?

LorenzMeier commented 3 years ago

Closing as stale.