Read an image with ADNS2610 optical sensor and Arduino Uno

Question

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):

Image handling

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:

ADNS2610 class

#ifndef ADNS2610_H
#define ADNS2610_H
#include <inttypes.h>
#include <Arduino.h>
//Addresses of the registers
#define CFG_REG_ADDR 0x00
#define STS_REG_ADDR 0x01
#define DY_REG_ADDR 0x02
#define DX_REG_ADDR 0x03
#define SQL_REG_ADDR 0x04
#define MXP_REG_ADDR 0x05
#define MNP_REG_ADDR 0x06
#define PXLSUM_REG_ADDR 0x07
#define PXLDAT_REG_ADDR 0x08
#define STRUPR_REG_ADDR 0x09
#define STRLWR_REG_ADDR 0x0A
#define INVPRD_REG_ADDR 0x0B
//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
class ADNS2610
{
 private :
 uint8_t _sclkPin;
 uint8_t _sdioPin; 
 protected :
 uint8_t ReadRegister( uint8_t address );
 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 );
 }
 ~ADNS2610( ) { };
 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
 *register's address (7 bits)*/
 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 );
}

ReadRegister function

uint8_t ADNS2610::ReadRegister( uint8_t address )
{
 address &= ~( 1 << 7 );//Specify data direction
 pinMode( _sdioPin, OUTPUT );
 /*Write the 1st byte: data direction (MSB)
 *and register's address*/
 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
 {
 frame[ i ] = ReadRegister( PXLDAT_REG_ADDR ) & 0b00111111;
 }
 SetNormalLED();
 return frame;
}

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

Update

See also this post where write and read operations are discussed in detail.

---

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

• Vogel612: 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 .

Answer 1

The pixel reading loop

 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.

• LRDPRDX: Thank you for your answer. I will modify the program according to your notes. About the last note ("side note") in your answer I really would like to ask a separate question about this.

Answer 2

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;
}

• LRDPRDX: 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.
• JDługosz: Why doesn’t it work?
• LRDPRDX: 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.
• JDługosz: I would have thought that generating fewer instructions and saving on register allocation is better than the number of characters typed.