# Preliminaries

I have optical sensor ADNS2610 (see datasheet here). There are a few problems which it can be used to solve, but for now let us focus on getting an image from that sensor. This sensor is kind of a small camera which has 400 cpi (counts per inch) resolution. And the resulting image consists of 324 = 18 x 18 pixels. The following is from the linked datasheet (p.23):

So in order to read a frame I should follow the algorithm:

1. Set the LED to forced awake mode
2. Write anything to the Pixel Data register
3. Read 324 times 6-bit unsigned integer from that register

So below is my program.

# Program

Object oriented approach and WriteRegister and ReadRegisters functions (slightly modified) are borrowed from this project by zapmaker.

Optical sensor is represented by a class:

#ifndef ADNS2610_H

#include <inttypes.h>

#include <Arduino.h>

//Bits of the configuration register
#define     CFG_REG_LED         0x00
#define     CFG_REG_PWRDWN      0x06
#define     CFG_REG_RST         0x07

//Bits of the Pixel data register
#define     PXLDAT_REG_SOF      0x07
#define     PXLDAT_REG_VLD      0x06

#define     N_PIXELS            324

{
private :
uint8_t         _sclkPin;
uint8_t         _sdioPin;

protected :
void            WriteRegister( uint8_t address, uint8_t data );

public :
ADNS2610( uint8_t sclkPin, uint8_t sdioPin ) :
_sclkPin( sclkPin ),
_sdioPin( sdioPin )
{
pinMode( _sclkPin, OUTPUT );
pinMode( _sdioPin, INPUT );
}

bool            IsAwake( );
signed char     GetDX( );
signed char     GetDY( );
uint8_t         GetSQUAL( );
uint8_t         GetMaxPixel( );
uint8_t         GetMinPixel( );
uint8_t         GetPixelAverage( );
uint8_t*        GetImage( );

void            SetAwakeLED( );
void            SetNormalLED( );
};

#endif


## WriteRegister function

void ADNS2610::WriteRegister( uint8_t address, uint8_t data )
{
address |= ( 1 << 7 );//Specify data direction

/*Transmit the 1st byte: data dir (MSB) and
pinMode( _sdioPin, OUTPUT );
for( int i = 7; i >= 0; i-- )
{
digitalWrite( _sclkPin, LOW );
digitalWrite( _sdioPin, address & ( 1 << i ) );
digitalWrite( _sclkPin, HIGH );
}

/*Transmit the 2nd byte*/
for( int i = 7; i >= 0; i-- )
{
digitalWrite( _sclkPin, LOW );
digitalWrite( _sdioPin, data & ( 1 << i ) );
digitalWrite( _sclkPin, HIGH );
}

/*Wait for 100 microseconds between write and either
*read or write operations (see p.15 of the datasheet)*/
delayMicroseconds( 100 );
}


uint8_t ADNS2610::ReadRegister( uint8_t address )
{
address &= ~( 1 << 7 );//Specify data direction

pinMode( _sdioPin, OUTPUT );

/*Write the 1st byte: data direction (MSB)
for( int i = 7; i >= 0; i-- )
{
digitalWrite( _sclkPin, LOW );
digitalWrite( _sdioPin, address & ( 1 << i ) );
digitalWrite( _sclkPin, HIGH );
}

pinMode( _sdioPin, INPUT );

/*Wait for 100 microseconds between address write operation
*and read data operation (see p.15 of the datasheet)*/
delayMicroseconds( 100 );

uint8_t data = 0;

/*Write the 2nd byte*/
for( int i = 7; i >= 0; i-- )
{
digitalWrite( _sclkPin, LOW );
digitalWrite( _sclkPin, HIGH );

data |= ( digitalRead( _sdioPin ) << i );
}

/*Wait for 0.25 microseconds between read and either
*read or write operations (see p.15 of the datasheet)*/
delayMicroseconds( 1 );

return data;
}


## GetImage function

uint8_t* ADNS2610::GetImage()
{
//Returns pixel map as an array
SetAwakeLED();//Step 1. in the above algorithm

WriteRegister( PXLDAT_REG_ADDR, 0x00 );//Step 2. in the above algorithm

static uint8_t frame[ N_PIXELS ];
for( int i = 0; i < N_PIXELS; i++ )//Step 3. in the above algorithm
{
}

SetNormalLED();

return frame;
}


As you can see I return image as just an array of uint8_t values from a function.

# Update

As always any critic, advice, correction and idea would be appreciated.

• Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. – Vogel612 Jun 8 '18 at 11:40

    for( int i = 0; i < N_PIXELS; i++ )


leaves me uncomfortable. The data sheet effectively says that some reads may return Data_Valid bit set to 0, and must be retried. Also, the only sure way to determine that the image is completed is to look at SOF bit.

The address setup functionality is shared between ReadRegister and WriteRegister. Consider factoring it out into its own (private) method.

While reading image into the static buffer is a technically valid decision, I recommend to read it into the caller provided buffer.

The // Write the second byte comment in ReadRegister is misleading. You are reading there.

As a side note, the clock stays low for a very short period of time. I don't know what the wire protocol is; if it is i2c, you'd want to monitor for clock to actually toggle high before reading. I don't know what the data sheet says about it, but I recommend to double check it.

Don't use #define for constants or "functions" (⧺ES.31).

⧺Enum.5 Don't use ALL_CAPS for enumerators and ⧺ES.9 Avoid ALL_CAPS names.

Writing identifiers with a leading underscore is a bad idea, and is generally dissuaded as a style. Note that it is legal for member names if (and only if) the next character is not a capital letter or another underscore.

   ~ADNS2610( ) { };


If you have a destructor that looks like this, don’t say it at all. The compiler knows it is “trivial” if you do not declare one. If you must mention it, you could declare it with =default but that still counts as a declaration and will suppress the generation of move semantics.

Many of the functions named “get…” seem like they are accessors. They should be const members. You have not used const anywhere in the code posted, so there are probably many places where it should be inserted.

address |= ( 1 << 7 );//Specify data direction


That might be clearer using binary literals, now that they are available. 0b10000000.

address & ( 1 << i )


Is the compiler smart enough to figure out that you shift the mask one position each time through the loop? It might generate better code as well as being clearer to state the intent more directly:

for (uint8_t mask= 0b1000'0000; mask; mask>>=1) { ⋯


(or 0x80 or a named constexpr — whatever is easier to read)

static uint8_t frame[ N_PIXELS ];
for( int i = 0; i < N_PIXELS; i++ )//Step 3. in the above algorithm
{
frame[ i ] = something
}


prefer to use the range-for.

for (auto& x : frame) {
x = something;
}

• Thank you for your answer. I have followed almost every your notices. Though, there is a little problem with using binary mask in for loop when reading a register. I cannot find simple and readable solution. – LRDPRDX Jun 9 '18 at 10:33
• Why doesn’t it work? – JDługosz Jun 9 '18 at 22:15
• I did not say that it didn't work. I just said that I could not find a solution that would be as short as using counter. There is no such a problem with writing because I have all bits at the beginning. – LRDPRDX Jun 10 '18 at 7:20
• I would have thought that generating fewer instructions and saving on register allocation is better than the number of characters typed. – JDługosz Jun 11 '18 at 2:45