Skip to content
This repository has been archived by the owner on Dec 6, 2018. It is now read-only.

I2C error handling #279

Merged
merged 6 commits into from
Sep 28, 2017
Merged

I2C error handling #279

merged 6 commits into from
Sep 28, 2017

Conversation

Dubland
Copy link
Collaborator

@Dubland Dubland commented Sep 11, 2017

No description provided.

irq::clear(irqn);
constexpr auto i2c = pick_i2c();

// Explain difference between Events and Flags ?????????
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can delete this line :)

Copy link
Collaborator

@forGGe forGGe left a comment

Choose a reason for hiding this comment

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

Prefix >#256 to each commit, not only 1st

// Now for any error interrupts was used common plan of action:
// 1) clear error flags
// 2) generate stop
// This may not work for some interrupt
Copy link
Collaborator

Choose a reason for hiding this comment

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

for some interrupts -> for some error states
to be more specific

// TODO: Create error handling for every error flags
I2C_GenerateSTOP(i2c, ENABLE);

// Software reset did not help with resolving the error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Be more verbose here; I propose use following form:

According to ... software reset (<function_name>) should help recover from error state, but when placed here it causes a hang 


// Explain difference between Events and Flags ?????????
// I2C_GetLastEvent() is suitable when multiple flags are monitored at the same time.
// Using the function I2C_GetFlagStatus() return the status of one single flag.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I asked you to explain that I2C_GetLastEvent() returns type which is compatible with I2C_FLAG_* definitions.

namespace ecl
{

static inline void bypass_puts(const char *str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing doxygen comment

if (rc == err::ok) {
i2c_dev::xfer();
// For full reset htu21d sensor need less than 15 ms as written in documentation
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is not relevant to the purpose of this function

rc = i2c_dev::xfer();
}
i2c_dev::unlock();
rc = try_xfer(0, data, sizeof(data));
Copy link
Collaborator

Choose a reason for hiding this comment

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

0 is a magic number. Add CMD_NONE to sensor I2C commands enum and use it instead of 0

@@ -214,10 +210,32 @@ err htu21d<i2c_dev>::write_user_register(uint8_t value)

i2c_dev::platform_handle::set_slave_addr(i2_addr);

err rc = try_xfer(tx_buff, nullptr, sizeof(tx_buff));

return rc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lines above can be combined

@forGGe forGGe mentioned this pull request Sep 11, 2017
@forGGe forGGe changed the title >#256 I2C error handling I2C error handling Sep 11, 2017
@forGGe
Copy link
Collaborator

forGGe commented Sep 11, 2017

Also, do not include >#256 in the PR name.

Copy link
Collaborator

@forGGe forGGe left a comment

Choose a reason for hiding this comment

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

err rc = i2c_dev::set_buffers(tx_buff, nullptr, sizeof(tx_buff));
err rc;

if (!cmd) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

-> cmd == CMD_NONE

// TODO: Create error handling for every error flags
I2C_GenerateSTOP(i2c, ENABLE);

// Accordint to I2C library software reset I2C_SoftwareResetCmd should help recover
Copy link
Collaborator

Choose a reason for hiding this comment

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

-> According

irq::clear(irqn);
constexpr auto i2c = pick_i2c();

// I2C_GetLastEvent() is suitable when multiple flags are monitored at the same time. Returns type is compatible with I2C_FLAG_* definitions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. line too long, split it according to code style
  2. Returns type is compatible -> The return type is

Copy link
Collaborator

Choose a reason for hiding this comment

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

i2c_dev::xfer();
}
i2c_dev::unlock();
err rc = try_xfer(cmd, &value, 1);

return rc;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

lines above can be combined

@forGGe forGGe merged commit bd2b3d1 into theCore-embedded:develop Sep 28, 2017
forGGe added a commit that referenced this pull request Sep 28, 2017
@forGGe
Copy link
Collaborator

forGGe commented Sep 28, 2017

Looks good to me, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants