Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX:GPS not working. Invalid values passed to px4_arch_configgpio #9665

Merged
merged 3 commits into from
Jun 14, 2018

Conversation

davids5
Copy link
Member

@davids5 davids5 commented Jun 13, 2018

This is the root cause of #9461 The _pins array was initialized to -1. It was used to index the _gpios array. The value at _gpios[-1] was a number that mapped to Analog mode on Port A pin 0. This is the UART4_TX pin and was being reconfigured by the fault in the camera_trigger to an analog input.

reverts hotfix

LorenzMeier
LorenzMeier previously approved these changes Jun 13, 2018
@LorenzMeier
Copy link
Member

Thanks! That's a very interesting find.

@davids5 davids5 requested a review from dagar June 13, 2018 22:36
@davids5
Copy link
Member Author

davids5 commented Jun 13, 2018

@dagar if the CI fail is tangential please merge this.

mhkabir
mhkabir previously approved these changes Jun 14, 2018
Copy link
Member

@mhkabir mhkabir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

px4_arch_configgpio(_gpios[_pins[i]]);
px4_arch_gpiowrite(_gpios[_pins[i]], !_polarity);
// Pin range is ranges from 1 to 6
if (_pins[i] > 0 && _pins[i] < (int)arraySize(_gpios)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I see this should be _pins[i] >= 0 && ...
Note the check in CameraInterfaceGPIO::trigger: it uses if (_pins[i] >= 0) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkueng Could you provide a final answer here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do we need to change this or not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, needs a change. _pins[i] is set to param value -1 in https://github.com/PX4/Firmware/blob/6f7b17d693a16ebb7a89210d2a7166f5f7c44b08/src/drivers/camera_trigger/interfaces/src/camera_interface.cpp#L42
and param value == 1 is a valid setting. Fix pushed.

@bkueng bkueng dismissed stale reviews from mhkabir and LorenzMeier via ef76e8b June 14, 2018 08:28
LorenzMeier
LorenzMeier previously approved these changes Jun 14, 2018
@davids5
Copy link
Member Author

davids5 commented Jun 14, 2018

@bkueng - Thank you for seeing and fixing that.

What do you think of a better approach for efficiency and simplicity? The parameter init phase should fill the array of pins with the actual GPIO values and end with 0. So the pin array would be the actual GPIO values. There would be no double indexing at run time and the loops end early on pin value of 0.

@bkueng
Copy link
Member

bkueng commented Jun 14, 2018

What do you think of a better approach for efficiency and simplicity?

Agreed. Can you do it? :)

@davids5
Copy link
Member Author

davids5 commented Jun 14, 2018

@bkueng yes.

David Sidrane and others added 3 commits June 14, 2018 14:29
   This reverts commit a62a71f.
   The root cause was the camera trigger passing invalid pin
   configuration setting overwriting the UART4 TX pin setting
   This is the root cause of #9461
   The _pins array was initialized to -1. It was used to index the
   _gpios array. The value at _gpios[-1] was a number that mapped to
   Analog mode on Port A pin 0. These is the UART4_TX pin and was
   being reconfigured by the fault in the camera_trigger to an
   alaog input.
Because _pins[i] is set from parameter value - 1
@dagar dagar merged commit 229b127 into master Jun 14, 2018
@dagar dagar deleted the master_gps_fix branch June 14, 2018 19:03
@davids5
Copy link
Member Author

davids5 commented Jun 14, 2018

@bkueng Refactoring is here #9680

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants