Skip to content

Conversation

@AarC10
Copy link
Member

@AarC10 AarC10 commented Aug 25, 2025

This closes #328

Description

Adds support for tenants designed to be run periodically, but without the overhead of using a task.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Testing with CpuMonitorTenant changes

Checklist:

  • New functionality is documented in the necessary spots (i.e new functions documented in the header)
  • Unit tests cover any new functionality or edge cases that the PR was meant to resolve (if applicable)
  • The CI checks are passing
  • I reviewed my own code in the GitHub diff and am sure that each change is intentional
  • I feel comfortable about this code flying in a rocket

@AarC10 AarC10 requested a review from a team as a code owner August 25, 2025 23:12
@AarC10 AarC10 requested a review from Copilot August 25, 2025 23:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for "Timed Tenants" - tenants designed to run periodically without the overhead of using a traditional task. The implementation provides a timer-based execution mechanism for tenant classes.

  • Introduces a new CTimedTenant class that extends CTenant with timer-based periodic execution
  • Adds a new constructor to CSoftTimer that automatically starts the timer upon initialization
  • Updates the build system to include tenant source files in the compilation

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
lib/f_core/os/tenants/c_timed_tenant.cpp Implementation of the timed tenant class with timer callback functionality
lib/f_core/os/CMakeLists.txt Updates build configuration to include tenant source files
include/f_core/utils/c_soft_timer.h Adds constructor that auto-starts timer with specified timeout
include/f_core/os/tenants/c_timed_tenant.h Header file defining the timed tenant interface
app/backplane/sensor_module/src/c_sensor_module.cpp Adds include for the new timed tenant header

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

Choose a reason for hiding this comment

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

From an abstraction point of view, does this deserve the name tenant?
Before now, tenants were things that were on tasks and managed by StartRTOS/StopRTOS
As far as I can tell, the timed 'tenant' is not run on a task nor is it started/stopped by Start/StopRTOS just by the constructor

Also, I believe the idea of the timer expiry function is that it is 'interrupt-esque' if not fully in an interrupt context
image
https://docs.zephyrproject.org/latest/kernel/services/timing/timers.html#using-a-timer-expiry-function

To me, Calling this a tenant and having the Run method on it disguises this limitation since all other Run's are not required to be interrupt safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

idk what the correct place to go from here is tbh,

  • Just documenting the limitation seems dangerous bc its really easy to not read documentatino and now our beautiful little abstraction isnt so conceptually solid bc some tenants arent tenatns
  • Idk the exact workings of this but if the timer just signalled to the task thats running the TimedTenant that would solve this, but, that kinda defeats the point of the timed scheduling that this was supposed to handle bc you're on a timer but still beholden to the loop on the task that runs its tenants and the delays between the runs

I think this needs a little more considering before it goes in

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair this was like a 15 minute thing to write

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.

Timed Software Tenant

3 participants