Skip to content

Conversation

@christopherwun
Copy link
Collaborator

  • Split these ringbuf + ads1x9x updates from bno08x PR
  • Copied Maria's comments on these files below

rb->write_idx = rb->read_idx = 0;
atomic_flag_clear(&rb->lock);

return rb;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(from Maria) Ensure read/write access to the allocated buffer is locked. Even if you’re handling it outside the API (prone to bugs, every caller must remember to lock), it’s safer to include locking within the API for future reuse. Consider also providing a _nolock variant so callers can lock once and perform multiple operations atomically without double-locking.
E.g:
'''
cionic_ringbuf_lock(&rb);
cionic_ringbuf_write_nolock(&rb, &b, len1);
cionic_ringbuf_write_nolock(&rb, &b, len2);
cionic_ringbuf_write_nolock (&rb, &b, len3);
cionic_ringbuf_unlock(&rb);
and
cionic_ringbuf_write(&rb, &b, len1); // this one will lock, write, and unlock
'''
If the buffer is touched in an ISR push to a queue and then in the task do the buffered writes with a mutex.

}

}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from Maria: See my earlier comment on locking the ring buffer.
As written, this code may block the ISR, too much is happening (memcpy in read_data, filtering, writing to the ring buffer). The ISR should be minimal; use a background callback to offload the work. I’ve attached example code (generated with ChatGPT) that shows how this can be done, and also some other suggestions it provided to make the code more efficient.

chatgpt_recommendations.txt

@christopherwun christopherwun merged commit 410c7f4 into main Sep 2, 2025
2108 of 2116 checks passed
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