Skip to content
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

Add TwoWire::writeReadAsync #2388

Merged
merged 10 commits into from
Sep 4, 2024
Merged

Conversation

qqqlab
Copy link
Contributor

@qqqlab qqqlab commented Aug 30, 2024

Whilst working on I2C sensors for flight controllers I noticed that the Wire lib does not have a method to do a combined async I2C write+read operation. Most sensors I work with require first to write a register number, followed by a read.

I was also missing a callback on async operation completion.

This PR adds two new public methods to provide this functionality:

bool writeReadAsync(uint8_t address, const void *wbuf, size_t wlen, const void *rbuf, size_t rlen, bool sendStop)

void onFinishedAsync(void(*function)(void))

In addition the dma channel initialization/destruction was moved out of the main read/write method. Before, each async read/write call claims dma channels and allocates buffers, and freeing them upon completion. Now, the claim is done on the first async read/write call, and calling end() will free the buffers and dma channels.

Also the 16 bit receive buffer was replaced with a direct dma write to the 8 bit user provided read buffer.

NB: Earle, many thanks for your hard work on this project!

Test Program

#define HW_PIN_I2C_SDA           20 //Wire: 0, 4(default), 8, 12, 16, 20   Wire1: 2, 6, 10, 14, 18, 26(default)
#define HW_PIN_I2C_SCL           21 //Wire: 1, 5(default), 9, 13, 17, 21   Wire1: 3, 7, 11, 15, 19, 27(default)

#include "Wire.h"

// QMC5883L sensor interface
#define ADR 0x0D

void QMC5883L_begin(){
    _writeReg(0x0B,0x01);
    QMC5883L_setMode(0x01,0x08,0x00,0X00); //Continuous, 100Hz, 2G, 512x OSR
}

void QMC5883L_setMode(uint8_t mode, uint8_t odr, uint8_t rng, uint8_t osr){
    _writeReg(0x09,mode|odr|rng|osr);
}
    
void _writeReg(uint8_t reg, uint8_t val){
    Wire.beginTransmission(ADR);
    Wire.write(reg);
    Wire.write(val);
    Wire.endTransmission();
}

int _readReg(uint8_t reg){
    Wire.beginTransmission(ADR);
    Wire.write(reg);
    Wire.endTransmission(false);
    Wire.requestFrom(ADR, (uint8_t)6);
    return Wire.read();
}

void setup() {
  Serial.begin(115200);
  
  Wire.setSDA(HW_PIN_I2C_SDA);
  Wire.setSCL(HW_PIN_I2C_SCL);
  Wire.setClock(100000);
  Wire.begin();

  QMC5883L_begin();
}

uint8_t d[256];
uint32_t t1; //start time
int dt; //call runtime
int dt2; //total runtime

void readRaw(){
    for(int i=0;i<6;i++) d[i]=0xAA;

    t1 = micros();
    Wire.beginTransmission(ADR);
    Wire.write(0x00);
    Wire.endTransmission(false);
    Wire.requestFrom(ADR, (uint8_t)0x06);
    Wire.readBytes(d, 6);
    dt = micros() - t1;
    dt2 = dt;
}

void readRawDma(){
    for(int i=0;i<6;i++) d[i]=0xBB;

    t1 = micros();
    Wire.beginTransmission(ADR);
    Wire.write(0x00);
    Wire.endTransmission(true);
    Wire.onFinishedAsync(nullptr);
    Wire.readAsync(ADR, d, 6, true);
    dt = micros() - t1;
    while(!Wire.finishedAsync());
    dt2 = micros() - t1;
}

void readRawDma2(){
    for(int i=0;i<6;i++) d[i]=0xCC;

    t1 = micros();
    uint8_t reg = 0x00;
    Wire.onFinishedAsync(readRawDma2_callback);
    Wire.writeReadAsync(ADR, &reg, 1, d, 6, true);
    dt = micros() - t1;
}

void readRawDma2_callback() {
  dt2 = micros() - t1;
  Serial.printf(" dt2=%4dus data=%02X%02X %02X%02X %02X%02X \n",dt2,d[1],d[0],d[3],d[2],d[5],d[4]);
}

void loop() {
  Serial.println();

  readRaw();
  Serial.printf("Blocking:     dt=%4dus dt2=%4dus data=%02X%02X %02X%02X %02X%02X\n",dt,dt2,d[1],d[0],d[3],d[2],d[5],d[4]);
  delay(250);

  readRawDma();
  Serial.printf("DMA polling:  dt=%4dus dt2=%4dus data=%02X%02X %02X%02X %02X%02X\n",dt,dt2,d[1],d[0],d[3],d[2],d[5],d[4]);
  delay(250);   

  readRawDma2();
  Serial.printf("DMA callback: dt=%4dus",dt);
  delay(250);
}

Output Test Program

Blocking:     dt= 894us dt2= 894us data=0ADE ECA5 181C
DMA polling:  dt= 243us dt2= 919us data=0AE8 ECA0 1826
DMA callback: dt=  18us dt2= 899us data=0AE3 EC96 1835

@earlephilhower
Copy link
Owner

Thanks for the contribution! Other than minor formatting issues w/CI this looks good after a quick glance. Let me read it more closely tomorrow, but we'll get this in soon!

Copy link
Owner

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick check and this breaks TalkingToMyselfAsync which is my go-to test since I don't have any simple SPI peripherals. It runs a single cycle and then fails to make any progress sending the 2nd message. Can you take a look at it?

@qqqlab
Copy link
Contributor Author

qqqlab commented Sep 1, 2024

Hmmm, not good. I'll have a look...

@qqqlab
Copy link
Contributor Author

qqqlab commented Sep 1, 2024

I've got [TalkingToMyselfAsync working. And my i2c test still works as well.

Apparently I was too aggressive moving the dma configuration from writeReadAsync() to beginAsync(), moving it back and adding dma_channel_cleanup solved it.

Also did some other minor cleanups.

@earlephilhower
Copy link
Owner

Great, thanks for the follow up!

Copy link
Owner

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just walked through this and wanted to say, very neat way of using the weird control-bits-per-byte in the I2C peripheral! This code itself looks good, but would you be able to put in a paragraph or two in docs/wire.rst with this new call and callback function? I could probably add it after the fact, but I think you have a better grasp of where this makes sense to use...

@qqqlab
Copy link
Contributor Author

qqqlab commented Sep 4, 2024

Thanks!

I've made an attempt to update the docs. Also, I renamed the function arguments to be more in line with the current naming and added sendStop = true as default.

Copy link
Owner

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for going the extra mile and adding some kind of docs! That's always the least fun but probably most important bit.

@earlephilhower earlephilhower merged commit bf33170 into earlephilhower:master Sep 4, 2024
20 checks passed
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.

2 participants