default value handling error

[zencuke]

All through begin() there are a series of case statements. In most of then a comment suggests that the author thinks the default switch case somehow relates to the application default value. In this code the default case covers user errors, when the user sets the switch variable to an unsupported value. That is not user friendly behavior. The code should either throw an error so the user can see and fix the problem or attempt to set something close to the user requested value.

For example accelSampleRate. Accepted values are 13, 26, 52 etc; power of 2 multiples of 13. If the user asks for 14, 21 or any other value not in the set they will silently get 104 Hz because that is the default case value for the switch, which of course makes no sense. That may be a reasonable value if the user doesn't specify a value which may be what the coder thought would happen but that is controlled by the initial (default) value of settings.accelSampleRate which happens to be 416. So it didn't even get the default value right. 104 is just a random meaningless value.

The user friendly options when the user asks for an unsupported value would be either:

1. Throw an error, print a message, do something so the user can see and fix the error.
2. Pick the supported value closest to what the user asked for.

I would prefer 1) but 2) is more beginner friendly.

Masking the error by moving forward with a randomly selected value (randomly selected by the coder) is not user friendly. At the very least it should be consistent and default to the actual default value the user would get by not specifying any value. Now we have 3 user cases, the user doesn't specify and gets the default of 416, the user asks for a supported value and gets it, or the user makes a typo, asks for an unsupported value, and gets 104.

[zencuke]

I might have coded this one as "switch(((settings.accelSampleRate/13)+1)*13)" which sort of uses the floor value.

[oclyke]

Hi @zencuke. Thanks for the issue and the suggestion, definitely helped to improve this code! I leaned toward option 1 where we allow the user to see if anything has changed. There is now an optional argument to the 'begin()' function that expects a pointer to a SensorSettings structure. That pointer will be filled out to match the '.settings' member of the IMU object when you called begin(), and then the .settings will be changed to reflect what was actually used in the default case. You can then compare the .settings fields with the fields in the pointed to structure as shown in an example.

If there's anything more you'd like say about this please feel free to re-open this issue.

Edit: commit 88488f9 addressed this issue