Make resetGlitchFilter less resource intensive

[martijnbastiaan]

This PR turned out to be slightly more involved than I anticipated: the current iteration doesn't simulate without initial values.

Still TODO:

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files

[DigitalBrains1]

Why did you make it symmetric?

the current iteration doesn't simulate without initial values.

Yeah, as discussed, I think we shouldn't try to fix that in this particular function; it is just only for platforms with initial values. Shall we make the function error out¹ both in simulation and HDL generation when its domain has InitBehavior Undefined?

¹ We could enforce this at the type level but all these constraints carry over to encompassing functions and it just gets annoying that the functions in your design accumulate all these constraints. That's why in similar situations we decided we'd rather error out (as long as we make sure we do that both in simulation and HDL generation). Though I'll admit that this particular function is often only found very near the top, so its constraints usually don't carry far :-).

[DigitalBrains1]

I already asked why you made it symmetric. I'm not sure it's the right thing to do, but if it is, could you document why you made it symmetric in the commit msg and I think also the Changelog?

[martijnbastiaan]

It seemed like the right thing to do. Do we want a glitch in the reset to immediately yoink it to the deasserted state? Edit: I looked around and it seems like glitch filters generally filter both ways. I've added the why to the changelog.

[DigitalBrains1]

Really? I thought it was more customary without the deassertion filter. Like, you don't want a reset to happen when the reset pulse is an unintended glitch. That's why you filter stuff that you deem too short.

Do we want a glitch in the reset to immediately yoink it to the deasserted state?

Why not? We've responded to the reset. Why would you keep the reset asserted longer? If you need long reset pulses, we have holdReset. A reset glitch filter prevents triggering a reset on a glitch.

Maybe it helps to consider the source of the glitches? Like, a button only glitches when you press it down or when you release it, not when you keep it pressed down or it is in the up position. In this case, all you need to filter is the repetition. Once it starts glitching, you know there's going to be a real change. But we want the glitch filter for more things than just button debouncing, it's just an example of glitches (in fact, the previous implementation with the shift register might be wholly unsuitable for button debouncing, waayy too long periods of bounce).

[edit]
Ah, you say

glitch filters generally filter both ways

which I can wholly agree to. But do reset glitch filters? That's a very specific type of glitch filter.
[/edit]

[martijnbastiaan]

I don't really mind either way. If you want me to change it I'll do it.

Why not? We've responded to the reset. Why would you keep the reset asserted longer?

It makes the implementation (every so slightly) more complex AFAICT if you need to account for asymmetry. But again, I don't really mind either way. Neither the increased hardware complexity nor the increased wait time (in context of pressing physical buttons) seem like real issues to me.

[DigitalBrains1]

If this were the addition of a glitch filter I would never have balked at the symmetric implementation. However, you changed an existing implementation to have different qualities, that's why my ears pricked up :-). I think the difference in the amount of LUTs is too small to consider relevant. By changing the deassertion, users might need to change their test suite for a design: a test runs a few cycles longer¹, hard-coded samples or hard-coded cycle numbers will need to be adapted. If a change is an improvement, that is perfectly fine. I'm not convinced here, kinda on the fence, but I'll let you make the call.

Are you changing the implementation to debounce a button? (By the way, the button is definitely not the only signal where a glitch actually always introduces a change in level.) We should probably also just have a button debouncing function, although there are so many possible implementations X-D. I think Gergő has one in his book? I've written one once some day that debounces a bunch of buttons at the same time. All buttons needed to stop bouncing before the bounce was considered settled, so there were cases where it was insufficient (really strange key rollover properties), but it did only require one counter for a group of buttons. Also, as soon as it saw a change in level it immediately reported that as the new state, but after that "the latch went opaque" until the signal settled.

¹ I'm reminded of one of our own test cases that had a hard-coded duration but the test was slightly changed without changing the duration and it no longer ran the last few samples, reducing the test coverage.

[martijnbastiaan]

The original documentation said:

It will then stay asserted for as long as the given reset was asserted consecutively.

Sooo.. the old implementation was symmetrical? I'm confused.

[martijnbastiaan]

Right, it would keep the reset asserted for at least n cycles guaranteed, but if a deasserted reset happened during that period (or after) it will immediately deassert. That's a bit harder to replicate, so I'm going to opt to make it fully symmetrical (and update the docs).

Thanks for the reviews!

[DigitalBrains1]

Right, that's a rather odd implementation. I forgot about that msb check during this discussion.

I think after the reset was asserted for n ¹ cycles consecutively, and then after n+k cycles a single unasserted sample occurs, it will be asserted for cycle numbers n through 2n+k give or take a sample (so for a duration of n+k, in other words, a length n is guaranteed).

Properly symmetrical sounds fine, and actually, many tests will respond the same, I think (since it will deassert n cycles after the reset is released, if the reset isn't actually glitchy in the test). That's awesome, if I had realised that earlier I would have been more positive! You can see it in the doctest; only the glitched deassertion changes.

¹ Configurable value for resetGlitchFilter.

[martijnbastiaan]

Good point, that's great.