StaticFIFO and normal FIFO #126

New Issue

muellerr · 2020-07-06T13:22:51+02:00

muellerr commented

2020-07-06 13:22:51 +02:00

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

muellerr referenced a pull request that will close this issue

2020-07-06 13:31:40 +02:00

FIFO is StaticFIFO now, new FIFO using vector #127

muellerr commented

2020-07-06 13:44:23 +02:00

I put the FIFOs in a fsfw namespace now.

muellerr commented

2020-07-07 17:56:51 +02:00

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)

muellerr commented

2020-07-11 11:48:57 +02:00

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.

gaisser closed this issue

2020-09-01 13:08:59 +02:00

Sign in to join this conversation.