Skip to content

Fix ambiguous error for .serialize() overloads#3707

Merged
thomas-bc merged 3 commits intonasa:develfrom
ethancheez:serialize-ambiguous-fix
Jun 10, 2025
Merged

Fix ambiguous error for .serialize() overloads#3707
thomas-bc merged 3 commits intonasa:develfrom
ethancheez:serialize-ambiguous-fix

Conversation

@ethancheez
Copy link
Contributor

@ethancheez ethancheez commented Jun 9, 2025 •
edited
Loading

Related Issue(s) N/A
Has Unit Tests (y/n) N
Documentation Included (y/n) N

Change Description

Add static_cast<U32> to the .serialize() calls within FileDownlink.cpp and FpySequencerRunState.cpp.

Rationale

Older versions of GCC (i.e. GCC 11.3.1) aren't that clever at picking one of the overloaded functions for .serialize(). So it gives the error:

fprime/Svc/FpySequencer/FpySequencerRunState.cpp:53:48: error: call of overloaded 'serialize(Fw::ComPacket::ComPacketType)' is ambiguous
   53 |     Fw::SerializeStatus stat = cmdBuf.serialize(Fw::ComPacket::FW_PACKET_COMMAND);
      |                                ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from fprime/Fw/Com/ComPacket.hpp:11,
                 from fprime/Svc/FpySequencer/FpySequencerRunState.cpp:2:
fprime/Fw/Types/Serializable.hpp:63:21: note: candidate: 'Fw::SerializeStatus Fw::SerializeBufferBase::serialize(U8)'
   63 |     SerializeStatus serialize(U8 val);  //!< serialize 8-bit unsigned int
      |                     ^~~~~~~~~
fprime/Fw/Types/Serializable.hpp:64:21: note: candidate: 'Fw::SerializeStatus Fw::SerializeBufferBase::serialize(I8)'
   64 |     SerializeStatus serialize(I8 val);  //!< serialize 8-bit signed int
      |                     ^~~~~~~~~
fprime/Fw/Types/Serializable.hpp:67:21: note: candidate: 'Fw::SerializeStatus Fw::SerializeBufferBase::serialize(U16)'
   67 |     SerializeStatus serialize(U16 val);  //!< serialize 16-bit unsigned int
      |                     ^~~~~~~~~

... etc.

I chose to cast as U32 based on unit tests. Fw.Com has a U32 context that causes the UT to fail if casted as an U8 (specifically FpySequencerTestMain.cpp:772.

Testing/Review Recommendations

I tested this on fprime-arduino toolchains since some of them use older GCC versions.

Review:
Make sure it is fine to cast as a U32.

Future Work

N/A

@codeflight1
Copy link
Contributor

codeflight1 commented Jun 9, 2025

Thanks for looking into this!

thomas-bc
thomas-bc requested changes Jun 9, 2025
View reviewed changes
@thomas-bc
Copy link
Collaborator

thomas-bc commented Jun 9, 2025

@zimri-leisher would you please review and confirm this is ok for FpySequencer ?

@thomas-bc thomas-bc requested a review from zimri-leisher June 9, 2025 17:45
@zimri-leisher
Copy link
Collaborator

zimri-leisher commented Jun 10, 2025

The compiler might be able to figure this out without an explicit cast if you changed the declaration of the ComPacketType from this:

            typedef enum {
                FW_PACKET_COMMAND, // !< Command packet type - incoming
                FW_PACKET_TELEM, // !< Telemetry packet type - outgoing
                FW_PACKET_LOG, // !< Log type - outgoing
                FW_PACKET_FILE, // !< File type - incoming and outgoing
                FW_PACKET_PACKETIZED_TLM, // !< Packetized telemetry packet type
                FW_PACKET_DP, //!< Data product packet
                FW_PACKET_IDLE, // !< Idle packet
                FW_PACKET_UNKNOWN = 0xFF // !< Unknown packet
            } ComPacketType;

To this

enum class ComPacketType : FwPacketDescriptorType {
...
}

That would be my preferred solution to this problem. However, the PR as it stands is acceptable, but I don't think it's the "future-proof" way to do it.

@thomas-bc
Copy link
Collaborator

thomas-bc commented Jun 10, 2025 •
edited
Loading

@zimri-leisher you're absolutely right, and indeed it actually simply doesn't work for now if you tried to switch FwPacketDescriptorType to a smaller value.

There is somewhat related work going on in devel...jwest115:fprime:issue-3529

Then, the CCSDS work will define that enum in FPP

@thomas-bc
Copy link
Collaborator

thomas-bc commented Jun 10, 2025

I'll go ahead and merge this since static_cast is the way it's handled in the core framework for now.

https://github.com/nasa/fprime/blob/devel/Fw/Com/ComPacket.cpp#L19

@thomas-bc thomas-bc merged commit ec777b8 into nasa:devel Jun 10, 2025
47 checks passed
@ethancheez ethancheez deleted the serialize-ambiguous-fix branch August 9, 2025 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomas-bc thomas-bc thomas-bc approved these changes

@zimri-leisher zimri-leisher Awaiting requested review from zimri-leisher

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ethancheez @codeflight1 @thomas-bc @zimri-leisher