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

stm32xx-i2c-server: record error code in ringbuf. #1330

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cbiffle
Copy link
Collaborator

@cbiffle cbiffle commented May 2, 2023

No description provided.

@cbiffle cbiffle requested a review from bcantrill May 2, 2023 22:08
Copy link
Collaborator

@bcantrill bcantrill left a comment

Choose a reason for hiding this comment

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

Looks good -- if you'll excuse me thinking aloud here, one of the annoyances about this particular ringbuf is that it is historically small because the memory usage is very close to power-of-two boundary and we haven't wanted to double its RAM consumption due to ringbufs. But this change doesn't increase the size, and seems unlikely to newly drown the ring buffer (with, say, alternating errors). And certainly, it gives us more understanding of what happened!

@cbiffle
Copy link
Collaborator Author

cbiffle commented Jun 6, 2023

Looks good -- if you'll excuse me thinking aloud here, one of the annoyances about this particular ringbuf is that it is historically small because the memory usage is very close to power-of-two boundary and we haven't wanted to double its RAM consumption due to ringbufs. But this change doesn't increase the size, and seems unlikely to newly drown the ring buffer (with, say, alternating errors). And certainly, it gives us more understanding of what happened!

So my sense from looking at ringbuf memory consumption across tasks recently is that we could probably save a bunch of memory by enabling more per-task I2C logging, and removing some of the central driver logging. The central driver logging is difficult to correlate with individual tasks, and gets rewritten quickly -- which is why we couldn't just look at it here!

I think that'd let us get away with less net RAM. But I'd want to play with it before committing.

@bcantrill
Copy link
Collaborator

FWIW, this has been addressed by #1413 -- and the ring buffer both expanded and made more space efficient in addition to recording the error code.

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