Skip to content

mode command: docs vs. implementation #16

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

Closed
ViennaMike opened this issue Nov 6, 2018 · 6 comments · Fixed by #52
Closed

mode command: docs vs. implementation #16

ViennaMike opened this issue Nov 6, 2018 · 6 comments · Fixed by #52

Comments

@ViennaMike
Copy link
Contributor

For the mode method, the docstring, and hence the API, read as if the user passes a text string to set a new mode. However, these text strings are defined at the top of the module, outside of the class, and use the circuit python const feature. One can work around it by looking up the mode vs. hex value and passing either an integer or hex, but that doesn't seem like the right solution. The most user-friendly mechanism would seem to be to support passing the text strings that correspond to the mode name. At a minimum, the docstring should be changed to show the mode name to hex mapping.

@siddacious
Copy link
Contributor

In some recent (and maybe not recent) drivers we've taken to putting things like this into an enum-like class (see here for an example) so you can refer to things like

Mode.CONFIG
Mode.ACCONLY
Mode.GYRONLY

I suggest we do the same and the docstring for the new class be written to include a table of the members and a description of each option.

@ViennaMike
Copy link
Contributor Author

Additional info: I see that I CAN use the mode names by specifying the module name. e.g., sensor.mode = adafruit_bno055.IMUPLUS_MODE, Maybe it's just me, but to my mind, that seems awkward, and in any case, docstring should reflect this.

@evaherrada
Copy link
Collaborator

@ViennaMike so are you proposing something closer to sensor.mode = 'ACCONLY_MODE' or sensor.set_mode('ACCONLY_MODE')?

@vkuehn
Copy link

vkuehn commented Oct 26, 2019

It would be great to have something like sensor.mode() which gives back or allows to set the current mode. Btw in which mode do you ship the bno055 ?

@tannewt
Copy link
Member

tannewt commented Nov 2, 2019

Note that sensor.mode is already a readable and writable property. There are two TODO items for this:

  • Add an example to the mode doc which makes it clear how to set it.
  • Add comments to each of the module constants (like CONFIG_MODE) so that they are included on ReadTheDocs.

No additional APIs need to be added. Only the documentation needs to be improved.

@vkuehn
Copy link

vkuehn commented Nov 13, 2019

oh great good to know !

@evaherrada evaherrada linked a pull request Jul 13, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants