rx <-> tx typo's, NULLing free'd variable, consistent CRITICAL sects #1180

stickbreaker · 2017-10-27T23:13:14Z

A couple of typos referencing tx_ring_buf when rx_ring_buf, slv_tx_mux
instead of slv_rx_mux.

Also, I2C_ENTER_CRITICAL()/I2C_EXIT_CRITICAL() usage was not consistent.
Only some of the set functions had them. Most of the get function
had them? It is my understanding that they should be wrapped around
writes, not reads?

Also, the ticks_to_wait timeout handling in i2c_master_cmd_begin() would
not handle integer rollover correctly.

A couple of typos referencing tx_ring_buf when rx_ring_buf, slv_tx_mux instead of slv_rx_mux. Also, I2C_ENTER_CRITICAL()/I2C_EXIT_CRITICAL() usage was not consistent. Only some of the _set_ functions had them. Most of the _get_ function had them? It is my understanding that they should be wrapped around writes, not reads? Also, the ticks_to_wait timeout handling in i2c_master_cmd_begin() would not handle integer rollover correctly.

CLAassistant · 2017-10-27T23:13:20Z

All committers have signed the CLA.

projectgus

Thanks for fixing the overflow issue. There are some stylistic changes requested, but we'll be happy to merge this.

projectgus · 2017-10-30T19:14:24Z

components/driver/i2c.c

@@ -258,6 +258,7 @@ esp_err_t i2c_driver_install(i2c_port_t i2c_num, i2c_mode_t mode, size_t slv_rx_
        }
    }
    free(p_i2c_obj[i2c_num]);
+		p_i2c_obj[i2c_num] = NULL;


Could you please space-ify the indentation here?

I have converted tabs to spaces where ever I could find them.
Do I need to recreate a pull? or will your merge pull the current from my fork?

Chuck.

projectgus · 2017-10-30T19:15:00Z

components/driver/i2c.c

@@ -634,14 +635,14 @@ esp_err_t i2c_set_period(i2c_port_t i2c_num, int high_period, int low_period)
 esp_err_t i2c_get_period(i2c_port_t i2c_num, int* high_period, int* low_period)
 {
    I2C_CHECK(i2c_num < I2C_NUM_MAX, I2C_NUM_ERROR_STR, ESP_ERR_INVALID_ARG);
-    I2C_ENTER_CRITICAL(&i2c_spinlock[i2c_num]);
+//    I2C_ENTER_CRITICAL(&i2c_spinlock[i2c_num]);


These critical sections can be removed instead of commented out.

(You're right that as there's no read-modify-write here the spinlock is unnecessary, thanks for pointing that out.)

I removed the commented old code.

projectgus · 2017-10-30T19:17:56Z

components/driver/i2c.c

@@ -1103,7 +1110,9 @@ esp_err_t i2c_master_cmd_begin(i2c_port_t i2c_num, i2c_cmd_handle_t cmd_handle,

    esp_err_t ret = ESP_FAIL;
    i2c_obj_t* p_i2c = p_i2c_obj[i2c_num];
-    portTickType ticks_end = xTaskGetTickCount() + ticks_to_wait;
+//    portTickType ticks_end = xTaskGetTickCount() + ticks_to_wait;
+// change to handle integer rollover correctly


You can remove these lines instead of commenting out.

Comments on why a change was made are usually best put into the commit message (you can split this PR into a few commits if this is easier for you, but that's optional.)

projectgus · 2017-10-30T22:06:17Z

components/driver/i2c.c

    // Wait event bits
    EventBits_t uxBits;
    while (1) {
-        TickType_t wait_time = (ticks_to_wait < (I2C_CMD_ALIVE_INTERVAL_TICK) ? ticks_to_wait : (I2C_CMD_ALIVE_INTERVAL_TICK));
+//        TickType_t wait_time = (ticks_to_wait < (I2C_CMD_ALIVE_INTERVAL_TICK) ? ticks_to_wait : (I2C_CMD_ALIVE_INTERVAL_TICK));
+				TickType_t wait_time = xTaskGetTickCounter();


I think this is a typo for xTaskGetTickCount() ?

space-ify, remove marker comments, correct awkward error wording.

fix type

stickbreaker · 2017-11-16T17:23:20Z

I have no Idea what changes were requested, I'm just going to close this pull and assume it was ignored.
Chuck.

projectgus · 2017-11-16T22:53:10Z

Hi @stickbreaker ,

Sorry, I've been travelling a lot the last few weeks and a few things have slipped through the cracks, including this. I apologise for ignoring you.

It looks like you addressed all the changes at the beginning of the month. I'll take another look today and get back to you, if that's OK.

projectgus · 2017-11-16T23:13:32Z

Changes look good. I've squashed the 3 commits into 1, with a couple of minor formatting changes, and pushed them into our internal merge queue.

Sorry again for the long delay. The PR will be updated again when the changes land.

stickbreaker · 2017-11-16T23:21:39Z

Sounds good.

Chuck.

projectgus · 2017-12-12T06:45:25Z

Hi @stickbreaker ,

Just a note to say we haven't forgotten this PR. Some notes popped up in review, so we've made some small tweaks (such as keeping the spinlocks on i2c_get_period, as it reads multiple registers to produce the result). Expecting this will be merged soon.

We've also combined #1353 into the same branch, as they touched similar functionality.

Angus

…ects A couple of typos referencing tx_ring_buf when rx_ring_buf, slv_tx_mux instead of slv_rx_mux. Also, I2C_ENTER_CRITICAL()/I2C_EXIT_CRITICAL() usage was not consistent. Only some of the _set_ functions had them. Most of the _get_ function had them? It is my understanding that they should be wrapped around writes, not reads? (I think we still need the lock for reading pairs of consistent values) Also, the ticks_to_wait timeout handling in i2c_master_cmd_begin() would not handle integer rollover correctly. Merges #1180

projectgus · 2017-12-18T05:08:38Z

Sorry for the extraordinary delay, cherry-picked as d913fff.

…ects A couple of typos referencing tx_ring_buf when rx_ring_buf, slv_tx_mux instead of slv_rx_mux. Also, I2C_ENTER_CRITICAL()/I2C_EXIT_CRITICAL() usage was not consistent. Only some of the _set_ functions had them. Most of the _get_ function had them? It is my understanding that they should be wrapped around writes, not reads? (I think we still need the lock for reading pairs of consistent values) Also, the ticks_to_wait timeout handling in i2c_master_cmd_begin() would not handle integer rollover correctly. Merges #1180

stickbreaker mentioned this pull request Oct 28, 2017

Add initial handling for long I2C reads. espressif/arduino-esp32#751

Merged

projectgus suggested changes Oct 30, 2017

View reviewed changes

stickbreaker added 2 commits October 30, 2017 21:22

convert Tabs to Space, Remove comments

7798146

space-ify, remove marker comments, correct awkward error wording.

type

fda63f7

fix type

stickbreaker closed this Nov 16, 2017

projectgus reopened this Nov 16, 2017

projectgus added the Status: Pending blocked by some other factor label Nov 16, 2017

projectgus approved these changes Nov 16, 2017

View reviewed changes

projectgus closed this Dec 18, 2017

igrr removed the Status: Pending blocked by some other factor label Aug 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rx <-> tx typo's, NULLing free'd variable, consistent CRITICAL sects #1180

rx <-> tx typo's, NULLing free'd variable, consistent CRITICAL sects #1180

Uh oh!

stickbreaker commented Oct 27, 2017

Uh oh!

CLAassistant commented Oct 27, 2017 •

edited

Loading

Uh oh!

projectgus left a comment

Uh oh!

projectgus Oct 30, 2017

Uh oh!

stickbreaker Oct 31, 2017

Uh oh!

projectgus Oct 30, 2017

Uh oh!

stickbreaker Oct 31, 2017

Uh oh!

projectgus Oct 30, 2017

Uh oh!

projectgus Oct 30, 2017

Uh oh!

stickbreaker commented Nov 16, 2017

Uh oh!

projectgus commented Nov 16, 2017 •

edited

Loading

Uh oh!

projectgus commented Nov 16, 2017

Uh oh!

stickbreaker commented Nov 16, 2017

Uh oh!

projectgus commented Dec 12, 2017

Uh oh!

projectgus commented Dec 18, 2017

Uh oh!

Uh oh!

rx <-> tx typo's, NULLing free'd variable, consistent CRITICAL sects #1180

rx <-> tx typo's, NULLing free'd variable, consistent CRITICAL sects #1180

Uh oh!

Conversation

stickbreaker commented Oct 27, 2017

Uh oh!

CLAassistant commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

projectgus left a comment

Choose a reason for hiding this comment

Uh oh!

projectgus Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

stickbreaker Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

projectgus Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

stickbreaker Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

projectgus Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

projectgus Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

stickbreaker commented Nov 16, 2017

Uh oh!

projectgus commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

projectgus commented Nov 16, 2017

Uh oh!

stickbreaker commented Nov 16, 2017

Uh oh!

projectgus commented Dec 12, 2017

Uh oh!

projectgus commented Dec 18, 2017

Uh oh!

Uh oh!

CLAassistant commented Oct 27, 2017 •

edited

Loading

projectgus commented Nov 16, 2017 •

edited

Loading