5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Vogel612 Jun 8 '18 at 11:40
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – LRDPRDX Jun 6 '18 at 11:48
2
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – LRDPRDX Jun 9 '18 at 10:33
  • \$\begingroup\$ Why doesn’t it work? \$\endgroup\$ – JDługosz Jun 9 '18 at 22:15
  • \$\begingroup\$ 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. \$\endgroup\$ – LRDPRDX Jun 10 '18 at 7:20
  • \$\begingroup\$ I would have thought that generating fewer instructions and saving on register allocation is better than the number of characters typed. \$\endgroup\$ – JDługosz Jun 11 '18 at 2:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.