StaticFIFO and normal FIFO #126

Closed
opened 2020-07-06 13:22:51 +02:00 by muellerr · 3 comments
Owner

I renamed the older FIFO to StaticFIFO (second template argument is the capacity).

The new FIFO takes the capacity as a ctor argument (so size can be determined at compile time and "optional" FIFO members are possible).

Both FIFOs are based on the FIFOBase, which simply takes a pointer to the first element and the maximum capacitry. FIFOBase also exposes the interface.

I split up FIFOBase in FIFOBase.tpp und FIFOBase.h und separate interface from implementation.

Idea: In most implementations, FIFO is actually named Queue. Maybe renamed FIFO / StaticFIFO to Queue and StaticQueue ? It Propably would be a good idea to also put them in a namespace then (e.g. fsfw) because Queue is quite a generic name.

I renamed the older FIFO to StaticFIFO (second template argument is the capacity). The new FIFO takes the capacity as a ctor argument (so size can be determined at compile time and "optional" FIFO members are possible). Both FIFOs are based on the FIFOBase, which simply takes a pointer to the first element and the maximum capacitry. FIFOBase also exposes the interface. I split up FIFOBase in FIFOBase.tpp und FIFOBase.h und separate interface from implementation. Idea: In most implementations, FIFO is actually named Queue. Maybe renamed FIFO / StaticFIFO to Queue and StaticQueue ? It Propably would be a good idea to also put them in a namespace then (e.g. fsfw) because Queue is quite a generic name.
muellerr added the
feature
label 2020-07-06 13:22:51 +02:00
Author
Owner

I put the FIFOs in a fsfw namespace now.

I put the FIFOs in a fsfw namespace now.
Author
Owner

As discussed, FIFOs not in namespace anymore, StaticFIFO is FIFO and new FIFO is DynamicFIFO.
There are actually some more containers which allocate dynamically (e.g. RingBuffer)

As discussed, FIFOs not in namespace anymore, StaticFIFO is FIFO and new FIFO is DynamicFIFO. There are actually some more containers which allocate dynamically (e.g. RingBuffer)
Author
Owner

There was actually a super evil bug in the new DynamicFIFO implementation which lead to undefined behaviour. I was looking 2h, first checking DHB changes, then checking CSB changes, then checking the polling task until I found out the issue was cause by the new DynamicFIFO.

Previous implementation:

template<typename T>
class DynamicFIFO: public FIFOBase<T> {
public:
	DynamicFIFO(size_t maxCapacity): FIFOBase<T>(values.data(), maxCapacity),
			values(maxCapacity) {};

private:
	std::vector<T> values;
};

#endif /* FRAMEWORK_CONTAINER_DYNAMICFIFO_H_ */

Calling .data() on the unconstructed vector is undefined behaviour. Very evil bug because it was hard to track down and (at least for me personally) very hard to spot.

The fix was to expose a setter function in FIFOBase for the internal data pointer and set that pointer AFTER the vector was constructed.

There was actually a super evil bug in the new DynamicFIFO implementation which lead to undefined behaviour. I was looking 2h, first checking DHB changes, then checking CSB changes, then checking the polling task until I found out the issue was cause by the new DynamicFIFO. Previous implementation: ```cpp template<typename T> class DynamicFIFO: public FIFOBase<T> { public: DynamicFIFO(size_t maxCapacity): FIFOBase<T>(values.data(), maxCapacity), values(maxCapacity) {}; private: std::vector<T> values; }; #endif /* FRAMEWORK_CONTAINER_DYNAMICFIFO_H_ */ ``` Calling .data() on the unconstructed vector is undefined behaviour. Very evil bug because it was hard to track down and (at least for me personally) very hard to spot. The fix was to expose a setter function in FIFOBase for the internal data pointer and set that pointer AFTER the vector was constructed.
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: fsfw/fsfw#126
No description provided.