Skip to content

Addition of FIFO Control and Status #9

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glenntamayo
Copy link

This library does not have methods to enable FIFO and select one of its modes.
The setFIFOMode will select one of the four available modes - Bypass, FIFO, Stream and Trigger.
The getFIFOMode will return the byte set in the FIFO_CTL register.
The getFIFOStatus will return the number of data values stored in FIFO.

@SDAMcIntyre
Copy link

It doesn't look like sparkfun have looked at this project in a while. :(

@ToniCorinne ToniCorinne requested a review from bboyho September 7, 2017 23:27
@nseidle
Copy link
Member

nseidle commented Mar 21, 2018

Hi Glenn - Sorry, someone should have contacted you a lot sooner. This looks like a good addition. I have a few changes I need to see.

The use of magic decimal numbers is not ideal. It would be a lot easier for me to read and check if you used bit wise operators such as (and this is pseudo code, I haven't read the datasheet in many years):

writeTo(ADXL345_FIFO_CTL, (1<<FIFO_MODE_STREAM));

Or something of the like.

And passing in a String to any function is (almost always) a bad idea. Using the String constructor will add a lot of overhead any time someone links in the library. I would prefer the user call a function called

myAccelerometer.enableStreamMode();

that in turn called your setFIFOmode with a define. Such as:

ADXL345::enableStreamMode()
{
    setFIFOMode(FIFO_MODE_STREAM);
}

Again, sorry we're so late to the party and thank you for your contribution. If you're interested in continuing this PR, please change it with the above recommendations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants