Fdcan use RAII for reference counting.

[cschuhen]

As mentioned #4223, this is a proposal for using RAII for the reference counting of alive sender/receiver/transceiver instances.

Note, this is a draft PR, and also includes #4271 as a dep.

@DrTobe

[DrTobe]

Nice to see that you are working on this one, too. But to be honest, I dislike that your solution adds more levels of indirection which makes it even harder to read and understand the code, which is already complicated enough IMHO. I have created an alternative approach (for now, only for FDCAN) which only adds two new types: TxGuard and RxGuard. Both contain the internal_operation function pointer (which we should really rename at some point!) and call it in the new method and in the Drop impl. Transceiver types just contain a (TxGuard, RxGuard) tuple. If I am not mistaken, this should do the trick (untested for now) without further complicating the code structure.

What do you think about it?

Edit: Sorry, I forgot to include the link 😄 https://github.com/embedded-rust-iml/embassy/tree/feature/raii-reference-counting-for-stm32-can

[cschuhen]

Yeah, I was a bit concerned about the extra code but I was also trying to optimise for memory usage. I think my approach didn't add to memory usage where yours adds extra memory usage to the RxTx types as well as any of the non-buffered senders. I guess it's a compromise either way, that's why I sought to get your thoughts before I made it non-draft, I didn't know how long to wait to see if you were still paying attention :). My opinion isn't too strong either way.

Possibly my patch can be improved to have the buffered 'senders' Just use the guarded info reference directly. Instead of also needing their own copy of waiter. This would reduce some of the additional code duplication/complexity. This would further increase consistency between how the buffered classes in 'common' and the main classes in 'not common'. This also means that the trait can be taken away... So then the 'Guard' classes would look a lot more like yours except for that they would old an info& instead of an internal_operation 'function pointer'. The complexity then lies in making sure that also works with bxcan :)

[DrTobe]

I think my approach didn't add to memory usage where yours adds extra memory usage

That seems to be true (for the non-buffered types) but we are talking about one function pointer per guard, i.e. only 4 bytes on STM32, if I am not mistaken. In my opinion, that's negligible and definitely acceptable in order to achieve lower code complexity.

Possibly my patch can be improved to have the buffered 'senders' Just use the guarded info reference directly. [...] The complexity then lies in making sure that also works with bxcan :)

In my first commit/approach, the guards contained an &fdcan::Info, so I was heading in the same direction. Then I realized that the split between fdcan::Info and bxcan::Info prevents us from using guards which contain &Info references and used the internal_operation function pointer fallback. Like this, I can use the same guard for buffered and non-buffered types.

For now, I guess you won't be able to let the buffered types use the guarded info directly, at least without massive code refactorings. But in the longer run, maybe it is even possible to do such a refactoring and reuse the same Info type. But right now, I can not evaluate if that is actually possible. I would leave that for another PR.

[cschuhen]

In my first commit/approach, the guards contained an &fdcan::Info, so I was heading in the same direction. Then I realized that the split between fdcan::Info and bxcan::Info prevents us from using guards which contain &Info references and used the internal_operation function pointer fallback. Like this, I can use the same guard for buffered and non-buffered types.

For now, I guess you won't be able to let the buffered types use the guarded info directly, at least without massive code refactorings. But in the longer run, maybe it is even possible to do such a refactoring and reuse the same Info type. But right now, I can not evaluate if that is actually possible. I would leave that for another PR.

Actually, this seems to not be the case. fdcan vs bxcan is based on a feature flag and the Info struct of both is quite similar. I didn't seem to have any issue in using the info struct directly.

Please check out a5eeec2

I guess it's somewhere in between the approaches. I will say, I'm struggled thinking about the best thing to call what was the &Info with RAII counting built in. As you saw, I was using InfoRef in my previous PR.

In general, the approach of your original PR a few weeks ago (to reference count not only buffered but the non-buffered CAN) has a bigger impact on bxcan as is... This is because some of the implementations, construct Rx or Tx elements for the purpose of individual write calls. This is not super ideal.

[DrTobe]

Actually, this seems to not be the case. fdcan vs bxcan is based on a feature flag and the Info struct of both is quite similar. I didn't seem to have any issue in using the info struct directly.

That's what I just haven't checked before. I expected that larger refactorings would be required. So using the Info struct directly is definitely the right way for the guards!

Since the guards do contain the Info struct now, I've built upon your most recent commits. I have updated the feature/raii-reference-counting-for-stm32-can branch with commit embedded-rust-iml@7473abd. There, I demonstrate how we can impl the Deref trait for the guard types (I have only done this for the combined Guard for now, TxGuard and RxGuard still missing, and I have only adjusted the FDCAN module). Having the Deref trait implemented, we can use the guard types as smart pointers transparently now in the module. Like this, we don't need to call the info() method whenever we want to access the Info struct.

If you want to go that way, there are still some things I would change and some open questions:

1. If we use the guards like this, I would not name the fields guard/guards but just info instead. From the usage perspective, the field behaves like the Info then.
2. Maybe, I would also prefer GuardedInfo, InfoGuard or similar (or your InfoRef) for the guard type to make clear that this type essentially is the Info field, just wrapped with a guard for reference counting.
3. You have replaced my (TxGuard, RxGuard) tuple by an essentially identical Guard struct. So the Guard struct holds the same &'static Info twice, once for each inner field. If desired, we could change that and let the Guard contain the &'static Info directly. This would add around the same amount of additional code. Do we want that?
4. Maybe, we can also use these ongoing changes now to rename the internal_operation function pointer and make clearer that this handles the reference counting.
5. I guess the internal_operation function pointer could take a &'static Info argument now and the function pointer could be moved out of the Info type. Then, in the guard types, we could change(self.info.internal_operation)(...) into super::internal_operation(self.info, ...). Since the internal_operation function pointer is currently generated multiple times via the impl_fdcan! macro, this would deduplicate there, too, and probably lead to smaller binary size.

[cschuhen]

Deref works great! I'm fine with all of your points. Ive pushed another couple of changesets that takes care of them.

I went with InfoRef. Seemed like being a Reference to Info is the most importnt part of what it did. I also found a couple of places where I missed removing a couple of the pre-RAII calls.

I renamed internal_operation. Actually, I tried to just make it an impl on Info(as a way to remove the duplication that you mentioned), which worked great for FDCAN but not for BXCAN because I didn't do the critical_section->Mutex-of-CriticalSection transition there (yet).

My changes are here: main...cschuhen:embassy:feature/raii-reference-counting-for-stm32-can

[DrTobe]

Great work!

Changing the internal_operation/adjust_reference_counter function into an impl on Info seems just right! I guess that will be possible if we use the critical section mutex approach in BXCAN, too?

One last remark: You have used the self.info.info() pattern again at some places where an InfoRef needs to be converted into a &'static Info. A more concise solution is to use &self.info or *self.info. Both use one of the deref coercion rules to achieve the required conversion.

[cschuhen]

Ok, thanks for that. I just finished now a conversion of BxCan to use the Mutex. This should allow me to finish adjust_reference_counter-as-info-Impl.

Thanks for chiming in on the Deref stuff. I'll look at fixing that up also.

[cschuhen]

OK, pushed as above with the move of adjust_reference_counter to impl Info.

[DrTobe]

That looks great, thanks for your effort!

The last thing I have spotted now is that we should probably rename the InternalOperation enum to RefCountOp or similar. Afterwards, I would consider this PR ready to merge.

[cschuhen]

So, I've pushed that rename and marked the PR as not-draft.
@lulf, @Dirbaio. There are a few changesets in this series. It does match our discussion here in the PR so I saw that as an advantage. Let me know if you want them condensed. Thanks