Improve backward compatibility [de]serialize[From/To] fallback on Fw::Serializable and Fw::Buffer by Kronos3 · Pull Request #3962 · nasa/fprime

Kronos3 · 2025-07-31T21:35:47Z

Related to #3944

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.

…mented

vincewoo

This looks good to me, thanks for fixing this!

bocchino · 2025-08-01T03:14:49Z

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 · 2025-08-01T03:22:03Z

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 · 2025-08-01T05:03:24Z

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 · 2025-08-01T06:28:56Z

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 · 2025-08-01T06:57:38Z

@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.

Fw/Types/Serializable.hpp

Co-authored-by: Rob Bocchino <bocchino@icloud.com>

LeStarch · 2025-08-01T16:14:20Z

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

New code comments
An FPP Alpha release

Is this accurate?

Kronos3 · 2025-08-01T16:19:08Z

Yes, here's how I understand it

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

Kronos3 added 2 commits July 31, 2025 12:52

Return UNIMPLEMENTED status on default F Prime serializables

1dd1430

Remove deprecation attr

09aa17a

Kronos3 mentioned this pull request Jul 31, 2025

Deprecate serialize/deserialize in favor of [de]serialize[From/To] nasa/fpp#766

Merged

3 tasks

Kronos3 added 4 commits July 31, 2025 15:36

Fix up time/dp

cabc6ec

Merge remote-tracking branch 'origin/devel' into serializable-unimple…

64db892

…mented

Split options for unimplemented into de/ser categories

aec9032

Clarify comment

60db7b2

Kronos3 requested review from LeStarch, thomas-bc and vincewoo and removed request for thomas-bc July 31, 2025 22:56

Kronos3 changed the title ~~Return UNIMPLEMENTED status on default F Prime serializables~~ Improve backward compatibility fallback on Fw::Serializable and Fw::Buffer Jul 31, 2025

Kronos3 changed the title ~~Improve backward compatibility fallback on Fw::Serializable and Fw::Buffer~~ Improve backward compatibility [de]serialize[From/To] fallback on Fw::Serializable and Fw::Buffer Jul 31, 2025

vincewoo approved these changes Jul 31, 2025

View reviewed changes

bocchino self-requested a review August 1, 2025 03:22

bocchino requested changes Aug 1, 2025

View reviewed changes

LeStarch previously approved these changes Aug 1, 2025

View reviewed changes

vincewoo mentioned this pull request Aug 1, 2025

After FPP updates to new v4.0 serialization methods, mark legacy serialization methods as DEPRECATED #3944

Closed

bocchino self-requested a review August 1, 2025 13:52

bocchino previously approved these changes Aug 1, 2025

View reviewed changes

Fw/Types/Serializable.hpp Outdated Show resolved Hide resolved

Update Fw/Types/Serializable.hpp

c26f979

Co-authored-by: Rob Bocchino <bocchino@icloud.com>

Kronos3 dismissed stale reviews from bocchino and LeStarch via c26f979 August 1, 2025 15:32

Merge branch 'devel' into serializable-unimplemented

1a99141

thomas-bc approved these changes Aug 1, 2025

View reviewed changes

thomas-bc merged commit bf7d986 into nasa:devel Aug 1, 2025
49 checks passed

Kronos3 deleted the serializable-unimplemented branch August 1, 2025 20:29

vincewoo mentioned this pull request Aug 5, 2025

(De)Serialization clean up of temporary workarounds #3971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Improve backward compatibility [de]serialize[From/To] fallback on Fw::Serializable and Fw::Buffer#3962
thomas-bc merged 8 commits intonasa:develfrom
Kronos3:serializable-unimplemented

Kronos3 commented Jul 31, 2025

Uh oh!

vincewoo left a comment

Uh oh!

bocchino commented Aug 1, 2025 •

edited

Loading

Uh oh!

bocchino commented Aug 1, 2025

Uh oh!

bocchino left a comment

Uh oh!

bocchino commented Aug 1, 2025

Uh oh!

bocchino commented Aug 1, 2025

Uh oh!

vincewoo commented Aug 1, 2025

Uh oh!

bocchino left a comment

Uh oh!

Uh oh!

LeStarch commented Aug 1, 2025

Uh oh!

Kronos3 commented Aug 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	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;
	}

Conversation

Kronos3 commented Jul 31, 2025

Uh oh!

vincewoo left a comment

Choose a reason for hiding this comment

Uh oh!

bocchino commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bocchino commented Aug 1, 2025

Uh oh!

bocchino left a comment

Choose a reason for hiding this comment

Uh oh!

bocchino commented Aug 1, 2025

Uh oh!

bocchino commented Aug 1, 2025

Uh oh!

vincewoo commented Aug 1, 2025

Uh oh!

bocchino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LeStarch commented Aug 1, 2025

Uh oh!

Kronos3 commented Aug 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bocchino commented Aug 1, 2025 •

edited

Loading