Skip to content

Conversation

@nathanjel
Copy link
Contributor

I have ported the STM VL53L1X ULD API (STSW-IMG009) and tested on MATEK F722PX board successfully. I hope this is useful. I know VL53L0X is supported, but I had only the VL53L1X in my drawer.

@digitalentity digitalentity added this to the 2.7 milestone Feb 26, 2021
Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

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

I understand that this is a direct and trivial port of ST's library, but I would still suggest refactoring the code to at least the following:

  • Remove functions that are never called
  • Make all functions that are not intended to be called from outside this file static to make it easier for the linker

/**
* @brief This function returns the SW driver version
*/
VL53L1X_ERROR VL53L1X_GetSWVersion(VL53L1X_Version_t *pVersion);
Copy link
Member

Choose a reason for hiding this comment

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

This is dead code, called from nowhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is addressed in most recent commit

/**
* @brief This function sets the sensor I2C address used in case multiple devices application, default address 0x52
*/
VL53L1X_ERROR VL53L1X_SetI2CAddress(busDevice_t * dev, uint8_t new_address);
Copy link
Member

Choose a reason for hiding this comment

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

Dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is addressed in most recent commit

static void vl53l1x_Init(rangefinderDev_t * rangefinder)
{
isInitialized = false;
VL53L1X_SensorInit(rangefinder->busDev);
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we check thet VL53L1X_SensorInit returned no error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is addressed in most recent commit

* @brief This function programs the interrupt polarity\n
* 1=active high (default), 0=active low
*/
VL53L1X_ERROR VL53L1X_SetInterruptPolarity(busDevice_t * dev, uint8_t IntPol);
Copy link
Member

Choose a reason for hiding this comment

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

Dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is addressed in most recent commit

@nathanjel
Copy link
Contributor Author

I understand that this is a direct and trivial port of ST's library, but I would still suggest refactoring the code to at least the following:

  • Remove functions that are never called
  • Make all functions that are not intended to be called from outside this file static to make it easier for the linker

Hi, thanks for reply. I agree and will update the commit.

@nathanjel nathanjel requested a review from digitalentity March 26, 2021 19:21
Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

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

LGTM

@DzikuVx DzikuVx merged commit 44c494a into iNavFlight:master Apr 2, 2021
@DzikuVx DzikuVx added the Release Notes Add this when a PR needs to be mentioned in the release notes label Apr 2, 2021
@nathanjel nathanjel deleted the feature/rangefinder_vl53l1x branch May 28, 2021 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release Notes Add this when a PR needs to be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants