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

I2C slave support; resolving conflicts against current master #5162

Closed
wants to merge 6 commits into from

Conversation

suculent
Copy link
Contributor

Replacing outdated/conflicting PR from #5161

@suculent suculent mentioned this pull request Sep 24, 2018
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

@suculent code wise I see only one detail I'd prefer to change (int->size_t). The rest is just style and cleanup.
I know this was originally PRd by @bjoham , but could you please handle the changes? Given that there will be no functional change, I think it can be merged right after that.

static volatile uint8_t twi_rxBufferIndex;

static void (*twi_onSlaveTransmit)(void);
static void (*twi_onSlaveReceive)(uint8_t*, int);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the second arg is the size of the first, it should be unsigned, preferably size_t.

twi_sda = sda;
twi_scl = scl;
pinMode(twi_sda, INPUT_PULLUP);
pinMode(twi_scl, INPUT_PULLUP);
twi_setClock(preferred_si2c_clock);
//pinMode(2, OUTPUT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want these commented lines left here, or should they be removed?


#if 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want this commented function here or should it be removed?

unsigned int i;
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
unsigned int reg;
for(i=0;i<v;i++)
reg = GPI;
for(i=0;i<v;i++) reg = GPI;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No single line for (please put the body on its own line)

#pragma GCC diagnostic pop
}

static bool twi_write_start(void) {
SCL_HIGH();
SDA_HIGH();
if (SDA_READ() == 0)
return false;
if (SDA_READ() == 0) return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No single line if please

return I2C_SCL_HELD_LOW; //SCL held low by another device, no procedure available to recover
int clockCount = 20;
uint8_t twi_status(){
if (SCL_READ()==0) return I2C_SCL_HELD_LOW; //SCL held low by another device, no procedure available to recover
Copy link
Collaborator

Choose a reason for hiding this comment

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

Single line if

return I2C_SCL_HELD_LOW_AFTER_READ; //I2C bus error. SCL held low beyond slave clock stretch time
while (SDA_READ()==0 && clockCount>0){ //if SDA low, read the bits slaves have to sent to a max
twi_read_bit();
if (SCL_READ()==0) return I2C_SCL_HELD_LOW_AFTER_READ; //I2C bus error. SCL held low beyond slave clock stretch time
Copy link
Collaborator

Choose a reason for hiding this comment

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

Single line if.

There are several more cases of ifs and fors, I'll skip pointing out the rest in the interest of sanity

}
}

#if 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want this #if 1, or can it be cleaned up?


// wait for stop condition to be exectued on bus
// TWINT is not set after a stop condition!
while (false) { //TWCR & _BV(TWSTO)){
Copy link
Collaborator

Choose a reason for hiding this comment

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

This while(false) makes no sense to me. Is it a remnant of something that didn't work as expected (I.e. comments)? Is there a ToDo here?

//digitalWrite(13, HIGH);
//switch(TW_STATUS){
switch(status) {
#if 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to keep this huge piece of commented out code, or can it be removed?

@suculent
Copy link
Contributor Author

Changes handled.

@suculent suculent closed this Oct 11, 2018
suculent pushed a commit to thinx-cloud/Arduino that referenced this pull request Oct 11, 2018
suculent pushed a commit to thinx-cloud/Arduino that referenced this pull request Oct 11, 2018
…Arduino into i2c_slave_by_bjoham

* 'i2c_slave_by_bjoham' of https://github.com/thinx-cloud/Arduino:
  cleaning up commit as requested by esp8266#5162 (review)
devyte pushed a commit that referenced this pull request Oct 26, 2018
* I2C slave support; resolving conflicts against current master

* removed unused argument, updateded to hopefully pass Travis

* cleaning up commit as requested by #5162 (review)

* cleaning up commit as requested by #5162 (review)

* type fix
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.

3 participants