[sx126x] Trust the code or the documentation?

[5frank]

I had some trouble with the sx126 driver and while browsing the code I've noticed there are things that is either different or missing in the documentation (SX1261/2 Data Sheet DS.SX1261-2.W.APP. found here: https://www.semtech.com/uploads/documents/DS_SX1261-2_V1.1.pdf)

So far I found the following:

• GetIrqStatus (OpCode=0x12) Documented as 3 bytes in the pdf, bits not documented at all. Only 2 bytes read in driver.

• REG_LR_PACKETPARAMS (0x0704) I do not find any reference to this register in data sheet.

Questions:

• Should I trust the data sheet or the driver (or none of them)? i.e. is the driver fully tested?
• Where did you get the missing information? Seems like there might be a document or a updated data sheet that should be published?

Possible related issue: https://github.com/Lora-net/LoRaMac-node/issues/601

[5frank]

Another thing not in manual but used in code: The CpuBusy flag in the GetStatus command (OpCode=0xC0)

[mluis1]

The GetIrqStatus documentation only defines 2 bytes. The IrqStatus field is 16 bits wide (2 bytes). These 2 bytes are indexed by index 2 and 3. The IrqStatus bits description is provided in table "Table 13-29: IRQ Registers".

The REG_LR_PACKETPARAMS register you are right it is not defined in the datasheet (we missed to document it). But, its MSB bit is where the HeaderType parameter of SetPacketParams API is stored. What we may do instead of updating the datasheet with this register is to handle the HeaderType like the PacketType

The CpuBusy bit will be removed from the drivers implementation as it has no real use. It will be noted as Reserved like in the datasheet.

The datasheet is currently undergoing on an update process. This as well as other issues are being updated/corrected.

The drivers have been tested as much as possible and are believed to be accurate.

We get the missing information from the Semtech R&D documents as well as by discussing with the persons that designed the hardware.

[5frank]

Thanks for clarifying that the documented "10-bit register called IRQ_reg" is the same as the 16-bit irqStatus. Still does not match the data sheet though, as there is one extra byte called Status. That is, if the following code is correct:

uint16_t SX126xGetIrqStatus( void )
{
    uint8_t irqStatus[2];

    SX126xReadCommand( RADIO_GET_IRQSTATUS, irqStatus, 2 );
    return ( irqStatus[0] << 8 ) | irqStatus[1];
}

... then OpCode 0x12 should instead be documented as:

Byte	0	1-2
Data from host	Opcode = 0x12	NOP
Data to host	RFU	IrqStatus(15:0)

[mluis1]

If you look at SX126xReadCommand function implementation you will notice that the status read is handled there.

All commands are implemented in the same way.

void SX126xReadCommand( RadioCommands_t command, uint8_t *buffer, uint16_t size )
{
    SX126xCheckDeviceReady( );

    GpioWrite( &SX126x.Spi.Nss, 0 );

    SpiInOut( &SX126x.Spi, ( uint8_t )command );
    SpiInOut( &SX126x.Spi, 0x00 ); // <-- status byte handling
    for( uint16_t i = 0; i < size; i++ )
    {
        buffer[i] = SpiInOut( &SX126x.Spi, 0 );
    }

    GpioWrite( &SX126x.Spi.Nss, 1 );

    SX126xWaitOnBusy( );
}

Please also note that the SX1261 is the radio that I currently use on a daily basis to develop/debug this project. We would already noticed if there was this kind of issues on the drivers.

[5frank]

Thank you for this information!!! This explains at least one of my problem with my porting attempt. I looked at other SX-driver board ports and did not expect the SX126xReadCommand to silently drop one byte. I also saw this which now looks like an error!?

RadioStatus_t SX126xGetStatus( void )
{
    uint8_t stat = 0;
    RadioStatus_t status;

    SX126xReadCommand( RADIO_GET_STATUS, ( uint8_t * )&stat, 1 );
    status.Value = stat;
    return status;
}

[mluis1]

I guess that you are right. I need to check with the designer (unfortunately he is not available this week).

Currently on this project this function is never called. This is maybe why we never noticed the potential issue.

As per the datasheet the function should be modified as follows:

RadioStatus_t SX126xGetStatus( void )
{
    uint8_t stat = 0;
    RadioStatus_t status;

    SX126xReadCommand( RADIO_GET_STATUS, ( uint8_t * )&stat, 0 );
    status.Value = stat;
    return status;
}

[5frank]

ok! I do not think that modification will work as it will always return 0. Either change SX126xReadCommand or use one of the other Read functions that do not drop one byte.

[mluis1]

Yes, it requires some more thoughts.

[mluis1]

Could you please check the provided fixes?

[5frank]

Seems alright to me! Side note: in the long run it would perhaps be better if the SX126xReadCommand and other similar functions was a simple wrapper for spi transfer(s) without any "knowledge" about the chip This would allow the same function(s) to be used for all chips and a lot of code could be deleted. Less code - less to maintain, single point of truth etc ...

[5frank]

Found another difference/bug:

    uint8_t buf[2] = { 0x00, 0x00 };
    SX126xWriteCommand(RADIO_CLR_ERROR, buf, 2);

Driver writes [opCode=0x07, 0x00, 0x00] but manual says [opCode=0x07, 0x00]

Do the command clear only provided bits to the opError register, as the code implies, or is all error bits cleared and only status byte received (as specified in data sheet)?