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

MS5611 failed to work on RPi on I2C bus #13814

Merged
merged 5 commits into from Dec 30, 2019

Conversation

SalimTerryLi
Copy link
Contributor

Describe problem solved by this pull request
Make MS5611 available on RPi with IIC bus.
MS5611 on /dev/i2c-1
start-up command: ms5611 start -I -T 5611

@SalimTerryLi
Copy link
Contributor Author

@dagar

@SalimTerryLi SalimTerryLi changed the title this commit made ms5611 works on RPi on I2C bus MS5611 failed to work on RPi on I2C bus Dec 30, 2019
@dagar dagar self-requested a review December 30, 2019 14:42
@dagar
Copy link
Member

dagar commented Dec 30, 2019

Nice work @SalimTerryLi.

Are the white space changes intentional (https://github.com/PX4/Firmware/pull/13814/files)?

if (bus.dev->init() != OK) {
PX4_ERR("MS5611 start failed");
delete bus.dev;
bus.dev = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

In c++ use nullptr in place of NULL.

return ret;
}

return probe();
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this is can and should be fixed lower level. In the NuttX version of the I2C wrapper I2C::init() calls probe. https://github.com/PX4/Firmware/blob/cc7a0cd69dec52db7816a24e58e782a82a8243a3/src/lib/drivers/device/nuttx/I2C.cpp#L140

I'll bring the posix version into sync. https://github.com/PX4/Firmware/blob/cc7a0cd69dec52db7816a24e58e782a82a8243a3/src/lib/drivers/device/posix/I2C.cpp#L74

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Can you delete this now and retest? #13818 has merged.

@SalimTerryLi
Copy link
Contributor Author

Nice work @SalimTerryLi.

Are the white space changes intentional (https://github.com/PX4/Firmware/pull/13814/files)?

I just did 'Reformat Code' in Clion...

Co-Authored-By: Daniel Agar <daniel@agar.ca>
@dagar
Copy link
Member

dagar commented Dec 30, 2019

Nice work @SalimTerryLi.
Are the white space changes intentional (https://github.com/PX4/Firmware/pull/13814/files)?

I just did 'Reformat Code' in Clion...

We use astyle with this configuration. https://github.com/PX4/Firmware/blob/master/Tools/astyle/astylerc

If it didn't already exist I would have gone with clang-format, but it's hard to transition from one tool to another without thousands of minor changes.

With astyle installed locally you can run make check_format or make format to use the in tree config.

@SalimTerryLi
Copy link
Contributor Author

Nice work @SalimTerryLi.
Are the white space changes intentional (https://github.com/PX4/Firmware/pull/13814/files)?

I just did 'Reformat Code' in Clion...

We use astyle with this configuration. https://github.com/PX4/Firmware/blob/master/Tools/astyle/astylerc

If it didn't already exist I would have gone with clang-format, but it's hard to transition from one tool to another without thousands of minor changes.

With astyle installed locally you can run make check_format or make format to use the in tree config.

I did 'Reformat code' before make format, Then I created the commit. Astyle may not deal with white spaces?

@dagar
Copy link
Member

dagar commented Dec 30, 2019

Confirmed ms5611 is still working on NuttX.

image

@dagar dagar merged commit 87e5da1 into PX4:master Dec 30, 2019
@SalimTerryLi SalimTerryLi deleted the pr-ms5611_driver_bug_rpi-fix branch February 29, 2020 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants