Improve backward compatibility [de]serialize[From/To] fallback on Fw::Serializable and Fw::Buffer

[Kronos3]

Related to #3944

We need this for nasa/fpp#766

In the FPP PR serialize/deserialize has been ported [de]serialize[From/To]. In F Prime Serializable, to keep backwards compatibility we have a fallback clause in the default serializable/buffer:

fprime/Fw/Types/Serializable.cpp

Lines 562 to 573 in 3dc73f9

	SerializeStatus SerializeBufferBase::deserializeTo(Serializable& val) {
	// Try new interface first, but if it returns FORMAT_ERROR (indicating default implementation),
	// fall back to old interface. This bridges auto-generated enums (old interface only)
	// with new serialization infrastructure.
	SerializeStatus status = val.deserializeFrom(*this);
	if (status == FW_DESERIALIZE_FORMAT_ERROR) {
	// Fallback to old interface for backward compatibility
	status = val.deserialize(*this);
	}
	return status;
	}

The issue is that FW_DESERIALIZE_FORMAT_ERROR is overloaded in use case for enums. When an enum deserialization is attempted and the value is not an enumerated constant in the enum type, the deserialization will return FW_DESERIALIZE_FORMAT_ERROR. The current fallback code will cause a double deserialization (the same happens in the serialization code).

This PR adds a new status code that disambiguates not implementing the deserialization with a format error.

[vincewoo]

This looks good to me, thanks for fixing this!

[bocchino]

Why are we providing a default deserializeTo that returns UNIMPLEMENTED and then catching UNIMPLEMENTED here? This seems convoluted. Wouldn't it be simpler to provide the implementation here as the default implementation of deserializeFrom? Is there some reason that won't work?

[bocchino]

Also I have concerns about adding a new enumeration to the F Prime serialization interface in order to support the code above, which looks like a temporary workaround for an inconsistency between F Prime and FPP.

[bocchino]

I would like to see my concerns addressed before this PR is merged.

[bocchino]

I think the problem is here:

fprime/Fw/Com/ComPacket.cpp

Lines 29 to 46 in 60db7b2

	SerializeStatus ComPacket::serializeTo(SerializeBufferBase& buffer) const {
	// Default implementation for base class - derived classes should override this method
	return FW_SERIALIZE_UNIMPLEMENTED;
	}
	SerializeStatus ComPacket::deserializeFrom(SerializeBufferBase& buffer) {
	// Default implementation for base class - derived classes should override this method
	return FW_DESERIALIZE_UNIMPLEMENTED;
	}
	// Deprecated methods for backward compatibility
	SerializeStatus ComPacket::serialize(SerializeBufferBase& buffer) const {
	return this->serializeTo(buffer);
	}
	SerializeStatus ComPacket::deserialize(SerializeBufferBase& buffer) {
	return this->deserializeFrom(buffer);
	}

We have deserialize calling deserialzeTo. This seems backwards, unless and until there is an FPP change. Without the FPP change, I think deserializeTo must be a convenience method that calls deserialize. It seems we are trying to change the interface without changing code that relies on the interface, and that is leading to a workaround and then another workaround to fix the first workaround.

[bocchino]

Maybe the way forward is to merge this PR, merge nasa/fpp#766, and then fix up F Prime to remove the workarounds introduced in #3880 and this PR, including the UNIMPLEMENTED serialization status. Is there a clear path to doing all that before the next release? If so, that would address my concerns.

[vincewoo]

@bocchino Yes, that is the plan that we discussed in the team meeting on Wednesday. The follow-up change to remove the work-arounds and DEPRECATE the legacy serialize and deserialize methods would be handled in #3944 . I will specifically note the UNIMPLEMENTED reversal in there now so we don't forget.

[bocchino]

Thanks, the comments here and in #3944 have addressed my concerns. Please note my suggested changes to the comments in the code.

[LeStarch]

@Kronos3 based on @bocchino's comments, I am expecting:

1. New code comments
2. An FPP Alpha release

Is this accurate?

[Kronos3]

Yes, here's how I understand it

1. Update code comment (done in c26f979)
2. Merge this PR
3. Merge Deprecate serialize/deserialize in favor of [de]serialize[From/To] fpp#766 & create FPP alpha
4. Vince to do After FPP updates to new v4.0 serialization methods, mark legacy serialization methods as DEPRECATED #3944 (comment)