Skip to content

Conversation

@heronet
Copy link
Contributor

@heronet heronet commented Oct 28, 2025

  • Add support for chaining multiple 32x16 panels horizontally by setting width to multiples of 32 (64, 96, 128, etc).
  • Fix a minor race condition issue.

}

k_sem_take(&data->lock, K_FOREVER);
k_mutex_lock(&data->lock, K_FOREVER);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mutex seemed like a better fit since we're protecting shared data access. Also helps if multiple threads end up calling display_write at different priorities

Copy link
Contributor

@rruuaanng rruuaanng Oct 29, 2025

Choose a reason for hiding this comment

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

It appears that the submitter of this document is you. I'm curious why your first choice wasn't a mutex, and now you're back to modify it again (just asking).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly a matter of preference actually. The initial implementation with a semaphore worked fine, and this patch primarily adds display chaining. While updating, I realized that a semaphore isn’t strictly necessary here, and using a mutex could also help with priority inversion. So I replaced the semaphore with a mutex.

Copy link
Contributor

@JarmouniA JarmouniA Oct 29, 2025

Choose a reason for hiding this comment

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

Expect for priority inversion mitigation, a semaphore is lighter than & recommended over a mutex in Zephyr. A mutex makes the function unusable from an ISR.
Also calling display_write from multiple threads would be a bad practice in my opinion.

https://academy.nordicsemi.com/courses/nrf-connect-sdk-fundamentals/lessons/lesson-8-thread-synchronization/topic/mutexes/
https://academy.nordicsemi.com/courses/nrf-connect-sdk-fundamentals/lessons/lesson-8-thread-synchronization/topic/semaphores/

Copy link
Contributor Author

@heronet heronet Oct 29, 2025

Choose a reason for hiding this comment

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

Yeah, it’s a trade-off. Do you think it would be better if I reverted to semaphores? I’m fine with that.

Edit: I went for semaphore as it is more performant and recommended than mutex.

@heronet heronet requested a review from JarmouniA October 29, 2025 05:57
- Add support for chaining multiple 32x16 panels horizontally by
  setting width to multiples of 32 (64, 96, 128, etc).

Signed-off-by: Siratul Islam <email@sirat.me>
- Add support for chaining multiple 32x16 panels horizontally by
  setting width to multiples of 32 (64, 96, 128, etc).
- Also fixed a race condition issue.

Signed-off-by: Siratul Islam <email@sirat.me>
@heronet heronet force-pushed the drivers/display/update-hub12-matrix-x branch from 69c4ee3 to 6fec537 Compare October 29, 2025 18:59
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants