hil: time: move Counter::set_overflow_client to new CounterOverflow trait

[stanciugabriel]

Pull Request Overview

This pull request resolves issue #4866 by moving hardware timer overflow support into a separate extension trait called CounterOverflow.

Previously, the main Counter trait forced unsupported hardware to include dummy overflow functions that could fail silently, like we saw in the earlgrey chip. Instead of using a runtime Result to patch this, we are now using the type system to guarantee support at compile time.

The main Counter trait is now completely free of overflow logic. The nrf5x chip explicitly implements the new CounterOverflow trait to preserve its hardware support. Also, all the dummy overflow code has been completely deleted from unsupported chips like esp32, apollo3, sam4l, msp432, stm32, and earlgrey. Finally, the trd105 documentation has been updated to reflect this new design.

Testing Strategy

Verified multiple boards using make check(including hail, apollo3, esp32-c3, earlgrey-cw310, and other relevant boards).

TODO or Help Wanted

n/a

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Formatting

• Ran make prepush.

AI Use

• The PR description details my use of AI in the production of the
  code in this PR, if any, and I have manually checked and
  personally certify the entire contents of this PR.
  No AI was used. AI was used for the documentation changes and commit message(wanted something more explicit because of the breaking changes).

[alexandruradovici]

Thank you for submitting this, first of all please detail how and if AI was used. Before we can merge this, I think we need to have a discussion about how we want to proceed with the issue.

[bradjc]

Do we have any examples of where the overflow client is actually used?

One fix would just be to delete the API. Perhaps it was a good idea at the time, but, in practice it isn't easy to support and we shouldn't build capsules on an API that most boards can't support.

Another fix would be to add a new CounterOverflow trait that is only implemented by chips that can support it.

I think this option, making a runtime check out of something that should never have compiled successfully in the first place, is something we should try to avoid. I can't think of any reasonable recourse to getting this error at runtime.

[alexandruradovici]

Do we have any examples of where the overflow client is actually used?

Based on this search https://github.com/search?q=repo%3Atock%2Ftock%20set_overflow_client&type=code, the earlgray and nrf5x chips seem to at least set a client.

Another fix would be to add a new CounterOverflow trait that is only implemented by chips that can support it.

Agree.

[stanciugabriel]

It seems like the API is not dead, at least not for the nrf5x chip:

tock/chips/nrf5x/src/rtc.rs

Lines 105 to 118 in dbdd349

	pub fn handle_interrupt(&self) {
	if self.registers.events_ovrflw.is_set(Event::READY) {
	self.registers.events_ovrflw.write(Event::READY::CLEAR);
	self.overflow_client.map(|client| client.overflow());
	}
	if self.registers.events_compare[0].is_set(Event::READY) {
	self.registers.intenclr.write(Inte::COMPARE0::SET);
	self.registers.events_compare[0].write(Event::READY::CLEAR);
	self.alarm_client.map(|client| {
	client.alarm();
	});
	}
	}
	}

tock/chips/nrf5x/src/rtc.rs

Lines 130 to 133 in dbdd349

	fn set_overflow_client(&self, client: &'a dyn time::OverflowClient) {
	self.overflow_client.set(client);
	self.registers.intenset.write(Inte::OVRFLW::SET);
	}

It stores an OverflowClient, enables the overflow interrupt in set_overflow_client(), and calls overflow() from handle_interrupt(). The earlgrey chip is a partial implementation that accepts a client, returns success, and then never uses it.

I don’t think deleting the API is the right move if we want to keep nrf5x functionality. I can pivot this PR to split overflow support into a separate trait, implemented only by chips that can actually generate overflow notifications, so support is explicit at compile time.

[stanciugabriel]

I've pushed the updates based on the feedback! The base Counter trait is now clean, nrf5x implements the new CounterOverflow extension trait, and all the dummy overflow code has been removed from the unsupported chips. Let me know if its all good.

[bradjc]

It makes sense we have something implement it, since we always try to have an example of any API we add. But if nothing is using overflow notifications, and something in the future could only use it on a single MCU family, I'm not sure what the value is of keeping it.

But, I do agree that what is implemented right now is a viable fix.

[bradjc]

We decided making a deprecated new trait is the right fix.

I don't like the TRD process (because this will break the book and just commits us to more work to ratify the TRD) but that isn't this PR's issue.

[bradjc]

We decided making a deprecated new trait is the right fix.

I don't like the TRD process (because this will break the book and just commits us to more work to ratify the TRD) but that isn't this PR's issue.