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

Some Basic Code improvements #1

Open
savejeff opened this issue Dec 19, 2022 · 1 comment
Open

Some Basic Code improvements #1

savejeff opened this issue Dec 19, 2022 · 1 comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement

Comments

@savejeff
Copy link

savejeff commented Dec 19, 2022

I found some code improvements:

in the header arduino::Stream& is used. some arduino cores do not have stream under the arduino namespace (teensy for example)
Stream is usually used directly:
void debug(Stream&);

Some functions do not return a value but should:


int BoschSensorClass::readAcceleration(float& x, float& y, float& z) {
  struct bmi2_sens_data sensor_data;
  bmi2_get_sensor_data(&sensor_data, &bmi2);
  x = sensor_data.acc.x;
  y = sensor_data.acc.y;
  z = sensor_data.acc.z;
  return 1;//<- this is missing
}
int BoschSensorClass::readGyroscope(float& x, float& y, float& z) {
  struct bmi2_sens_data sensor_data;
  bmi2_get_sensor_data(&sensor_data, &bmi2);
  x = sensor_data.gyr.x;
  y = sensor_data.gyr.y;
  z = sensor_data.gyr.z;
  return 1; //<- this is missing
}

// Magnetometer
int BoschSensorClass::readMagneticField(float& x, float& y, float& z) {
  struct bmm150_mag_data mag_data;
  bmm150_read_mag_data(&mag_data, &bmm1);
  x = mag_data.x;
  y = mag_data.y;
  z = mag_data.z;
  return 1;//<- this is missing
}

also:

  • there is no need to declare all functions virtual if no class inheritance is used
  • the end() and getTemperature() function are declared in the .h file but is missing in the .cpp
  • the same is true for interrupt_handler if mbed is not defined as it is only removed in the .cpp by ifdef but is always defined in the .h
  • in begin, there is no (working) check for chip id. so begin can not fail. it would be good to check the chip id reg (0x00) for value 0x24 to make sure an imu is actually connected to I2c. same goes for the mag device id
  • the debug print result only uses the bmi270 result codes but is also called for results of the BMM150. there should be a second print results for bmi result codes like BMM150_E_NULL_PTR, BMM150_E_DEV_NOT_FOUND etc
  • i2c reads should have the following structure:

beginTransmission
write
endTransmission(false) <- this is important
requestFrom(...)
read
endTransmission() <- only end transmission after interaction is done


@aentinger
Copy link
Contributor

Hi @savejeff ☕ 👋

Sorry about getting back to you only at this time, but the library has escaped my notice.

Can you come up with a PR containing your proposed fixes?

@aentinger aentinger added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

No branches or pull requests

2 participants