mma8652: add support for all mma8x5x accelerometers#5433
mma8652: add support for all mma8x5x accelerometers#5433PeterKietzmann merged 1 commit intoRIOT-OS:masterfrom
Conversation
|
Won't review until next Hack'n'ACK. If anyone volunteers, go ahead! |
|
There is also ./tests/driver_mma8652 that needs to be adapted. |
|
@vincent-d thx and ACK |
| * @return -5 if accelerometer configuration failed | ||
| */ | ||
| int mma8652_init(mma8652_t *dev, i2c_t i2c, uint8_t address, uint8_t dr, uint8_t range); | ||
| int mma8652_init(mma8652_t *dev, i2c_t i2c, uint8_t address, uint8_t dr, uint8_t range, uint8_t type); |
There was a problem hiding this comment.
Don't you think it's a bit confusing to stay with the mma8652 while changing the driver to mma8x5x? Was it because of the API change?
There was a problem hiding this comment.
I agree it can be confusing.
I did not want to do the full API change to avoid breaking compatibility. But we can imagine a couple of macro to translate calls if needed.
So I can still make the change API if you prefer it.
|
What is the difference between these sensors actually? |
|
@PeterKietzmann There is no common datasheet for the whole family, so it's not so easy to point out every difference. Some have tap detection, others freefall detection, or the output precision can differ. But from what I saw, the subset of functionalities implemented in this driver is common to all devices. Only the device id changes. |
|
Currently I don't know which way to go for merging sensor drivers. #5484 shows another way for that. I'd like to hear @haukepetersen's opinion on that. |
|
We use a mma8653 which works the same but has a different device ID, and I didn't want to do a big api change without having a feedback. I thought at this time this was the simplest way to support it. |
|
Well, I don't have a design decision by hand. Is it urgent for you to get this feature in? Otherwise I tend to give it some more time and find a good way to merge similar sensor drivers |
|
I would not say it's really urgent, but I would rather have that merged and then spend a bit of time later on to refactor with a new design than just wait. The changes are pretty small anyway. It's up to you ;) |
|
Then let's try to get this merged for 2016.10. |
|
@miri64 the change itself is ok -> "mergable". The question is about how to combine device drivers for "similar" devices. Anyway, I won't NACK this one. But please decide you! |
|
I would say that is something we always can decide later on. Wouldn't be the first time that consolidation happened, after two similar features were merged ;-) (see |
|
Feature freeze -> postponed |
|
Ping ? |
|
Just saw that this PR implies an API change. Anyone affected by this??? Otherwise I would approve and merge this PR soon. |
|
@vincent-d would you squash your second commit please? |
59d3a79 to
b9b838f
Compare
|
@PeterKietzmann squashed |
PeterKietzmann
left a comment
There was a problem hiding this comment.
Enough time for objections. ACK and go. I will add the memo label cause we should find a way to better handle similar device drivers and adapt this one accordingly.
This adds the support of all mma8x5x accelerometers.
The device id is the only difference between the devices with the current driver.
Backward compatibility should work as the MMA8652 has the id 0 in the enum.