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

Added depth and pressure, updated README #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

808brick
Copy link

@808brick 808brick commented Feb 15, 2022

Hello, I have used the C++ version of this KellerLD module that is used for microcontrollers:
https://github.com/bluerobotics/BlueRobotics_KellerLD_Library/

That library features calculations to convert the pressure obtained from the sensor to Depth and Altitude readings. I was hoping this feature could be added to this module, as we were utilizing this python module for use with Jetson Nano's, which have a GPIO pinout to read in the values from the BlueRobotics sensor.

I made modifications to the code, and tried to keep similar naming conventions / styles to that of your Python setup, as well to reference the way you have does the calculations in the C++ version of this module.

Things that were added/changed:

  • sensor.pressure() function can now take in a string of 'Pa', 'bar', 'mbar', to change the unit of pressure returned from the sensor. Defaults to 'bar', and will throw an assertion exception if an invalid unit is given.
  • New sensor.depth() and sensor.altitude() functions which basically copies the functionality of the C++ version of this module.
  • Updated README so people are aware of the added functionality mentioned in the previous 2 points
  • Added a sensor.set_fluid_density() function, similar to that of the C++ version.
  • Updated example.py to include the new functions, and to break the while loop properly when Ctrl + C is pressed.

The modified code was tested on a Jetson Nano (by running the modified example.py script, and changing the pressure units) and seems to perform as expected.

I'm open to suggestions / changes to better match your formatting preference, but figured this would be useful for people in the future so they do not have to do their own conversions / calculations, especially if they are in a situation like me where they migrate from the functionality in the C++ microcontroller module, to this Python version.

-Raymond

Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

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

Hi, just saw this! Thanks for submitting these changes :-)

Mostly looks good, just a couple of suggestions / things to consider :-)


def __init__(self, bus=1):
self._bus = None
self._fluid_density = 1029 # kg/m^3
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably use set_fluid_density (e.g. in case someone changes the function to include logging later), and should probably also have a parameter in the __init__ definition (e.g. def __init__(self, bus=1, fluid_density=1029):) so it's settable directly on object instantiation.


def set_fluid_density(self, fluid_density):
'''
Provide the density of the working fluid in kg/m^3. Default is for
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing, because the fluid_density parameter has no default value - the "default" mentioned in the docstring is instead a default from class instantiation (which is outside the scope of this function, and likely isn't relevant to mention here).

It may also be worth adding a few string-based defaults, either just 'fresh'->997 and 'sea'->1029, or could make use of wikipedia's salinity levels by water type overview. Not essential, just could be nice to have (although those levels could also be implemented separately in a Water class or something, which may be more appropriate).

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.

2 participants