I2C abort reason #1370

> assigning to myself; i think i'd rather return the abort reason in the top half of a 64 bit rc (and do some API massaging) I don't think you liked this change? I could add a variant of the functions to return int64_t if that sounds like what you wanted?

kilograham commented

2024-07-23 11:21:25 -05:00

(Migrated from github.com)

yeah, i guess returning a struct with two elements in a replacement APIs and picking the result out for the existing APIs is good
(Note a two uint32_t struct will be returned correctly in r0/r1 i believe)

yeah, i guess returning a struct with two elements in a replacement APIs and picking the result out for the existing APIs is good (Note a two uint32_t struct will be returned correctly in r0/r1 i believe)

lurch (Migrated from github.com) reviewed

2024-09-25 18:42:09 -05:00

src/rp2_common/hardware_i2c/include/hardware/i2c.h

Does this (and the other new _result_ functions) need to be added to the header file after the old functions, in order not to invalidate the documented doxygen \return parameter?

Does this (and the other new `_result_` functions) need to be added to the header file _after_ the old functions, in order not to invalidate the documented doxygen `\return` parameter?

lurch (Migrated from github.com) reviewed

2024-09-25 18:44:50 -05:00

src/rp2_common/hardware_i2c/i2c.c

					
				@ -130,6 +130,15 @@ void i2c_set_slave_mode(i2c_inst_t *i2c, bool slave, uint8_t addr) {

				    i2c->hw->enable = 1;

For consistency with function-names and return-types, I wonder if i2c_write_blocking_internal ought to be renamed to i2c_write_result_blocking_internal ? 🤔 (It's an internal-only function name, so I think we're free to rename it?)

For consistency with function-names and return-types, I wonder if `i2c_write_blocking_internal` ought to be renamed to `i2c_write_result_blocking_internal` ? :thinking: (It's an internal-only function name, so I think we're free to rename it?)

peterharperuk (Migrated from github.com) reviewed

2024-11-11 06:43:40 -06:00

src/rp2_common/hardware_i2c/i2c.c

					
				@ -130,6 +130,15 @@ void i2c_set_slave_mode(i2c_inst_t *i2c, bool slave, uint8_t addr) {

				    i2c->hw->enable = 1;

out of date

peterharperuk commented

2024-11-11 06:47:44 -06:00

(Migrated from github.com)

I've had another go at this. I decided I couldn't face adding new overloads to an already complicated API, just to get a not-often used reason for an abort. Instead I've added a new PICO_ERROR_ABORT error code. This is only used if PICO_I2C_RETURN_ABORT_REASON=1 in which case the abort reason is taken from this. So you can obtain the reason with a bit of maths.

if (ret <= PICO_ERROR_ABORT)
    abort_reason = PICO_ERROR_ABORT - ret; // one of I2C_IC_TX_ABRT_SOURCE_ABRT_*_LSB

I've had another go at this. I decided I couldn't face adding new overloads to an already complicated API, just to get a not-often used reason for an abort. Instead I've added a new PICO_ERROR_ABORT error code. This is only used if PICO_I2C_RETURN_ABORT_REASON=1 in which case the abort reason is taken from this. So you can obtain the reason with a bit of maths. ``` if (ret <= PICO_ERROR_ABORT) abort_reason = PICO_ERROR_ABORT - ret; // one of I2C_IC_TX_ABRT_SOURCE_ABRT_*_LSB ```

kilograham commented

2024-11-11 10:16:38 -06:00

(Migrated from github.com)

interesting... i think perhaps i'd prefer to make something more generic (and not overlaying existing/future return codes)...

e.g. assign codes 0x80000000 to 0xbfffffff as "data" error codes which return 30 bits of function specific result (similar to the way that any positive value can be success)

can then add a pico_error_has_data(code) and pico_error_get_data(code) macros... the I2C API can add a more specific API that uses this

interesting... i think perhaps i'd prefer to make something more generic (and not overlaying existing/future return codes)... e.g. assign codes `0x80000000` to `0xbfffffff` as "data" error codes which return 30 bits of function specific result (similar to the way that any positive value can be success) can then add a `pico_error_has_data(code)` and `pico_error_get_data(code)` macros... the I2C API can add a more specific API that uses this

peterharperuk commented

2024-11-11 10:26:43 -06:00

(Migrated from github.com)

Sounds good, will have a go at that. The one issue is that the docs specifies that PICO_ERROR_GENERIC is returned if there's no ack (abort basically), that's the reason I haven't enabled this by default as it's basically an API breakage?

kilograham commented

2024-11-11 10:43:35 -06:00

(Migrated from github.com)

yes; we have hit this elsewhere - hard to change from -1, though we want to move people towards < 0 ..
so yeah, you can still leave this optional

yes; we have hit this elsewhere - hard to change from -1, though we want to move people towards `< 0` .. so yeah, you can still leave this optional

lurch (Migrated from github.com) reviewed

2024-11-11 20:01:24 -06:00

src/common/pico_base_headers/include/pico/error.h

					
				@ -44,2 +44,3 @@

				    PICO_ERROR_VERSION_MISMATCH = -20,          ///< A version mismatch occurred (e.g. trying to run PIO version 1 code on RP2040)

				    PICO_ERROR_RESOURCE_IN_USE = -21            ///< The call could not proceed because requires resourcesw were unavailable

				    PICO_ERROR_RESOURCE_IN_USE = -21,           ///< The call could not proceed because requires resourcesw were unavailable

				    PICO_ERROR_ABORT = -22,                     ///< The call could not proceed so was aborted

nitpick: "requires resourcesw" -> "required resources"

lurch (Migrated from github.com) reviewed

2024-11-11 20:02:18 -06:00

src/rp2_common/hardware_i2c/i2c.c

					
				@ -130,6 +130,15 @@ void i2c_set_slave_mode(i2c_inst_t *i2c, bool slave, uint8_t addr) {

				    i2c->hw->enable = 1;

				}

				// PICO_CONFIG: PICO_I2C_RETURN_ABORT_REASON, change i2c functions to return the abort reason via a return code less than or equal to PICO_ERROR_ABORT, type=bool, default=0, group=harware_i2c

harware_i2c -> hardware_i2c

`harware_i2c` -> `hardware_i2c`

This pull request has changes conflicting with the target branch.

src/common/pico_base_headers/include/pico/error.h

View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Merge commit title

Merge pull request 'I2C abort reason' (#1370) from peterharperuk/add_last_abort_reason into develop

Merge commit body

Reviewed-on: https://git.richey.dev/gh-raspberrypi/pico-sdk/pulls/1370

Merge pull request 'I2C abort reason' (#1370) from peterharperuk/add_last_abort_reason into develop

Reviewed-on: https://git.richey.dev/gh-raspberrypi/pico-sdk/pulls/1370

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'peterharperuk/add_last_abort_reason:peterharperuk/add_last_abort_reason'

git switch 'peterharperuk/add_last_abort_reason'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'develop'

git merge --no-ff 'peterharperuk/add_last_abort_reason'

git switch 'peterharperuk/add_last_abort_reason'

git rebase 'develop'

git switch 'develop'

git merge --ff-only 'peterharperuk/add_last_abort_reason'

git switch 'peterharperuk/add_last_abort_reason'

git rebase 'develop'

git switch 'develop'

git merge --no-ff 'peterharperuk/add_last_abort_reason'

git switch 'develop'

git merge --squash 'peterharperuk/add_last_abort_reason'