Add Lossless Data Product Compression Components#5235
Conversation
…k the container size
| static_cast<FwAssertArgType>(ctx->zlib_alloc_buffer.getSize())); | ||
| const FwSizeType free_space = ctx->zlib_alloc_buffer.getSize() - ctx->bump_allocator; | ||
|
|
||
| const FwSizeType alloc_size = items * size; |
| // Component construction and destruction | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| DpCompressProc ::DpCompressProc(const char* const compName) : DpCompressProcComponentBase(compName) {} |
|
|
||
| DpCompressProc ::DpCompressProc(const char* const compName) : DpCompressProcComponentBase(compName) {} | ||
|
|
||
| DpCompressProc ::~DpCompressProc() {} |
|
|
||
| } | ||
|
|
||
| Svc::CompressionAlgorithm DpZLibCompressor ::compressChunk_handler(FwIndexType portNum, |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Coverage report — base
|
| Module | Line | Δ | Function | Δ | Branch | Δ |
|---|---|---|---|---|---|---|
Fw/DataStructures |
98.21 | -0.09 | 97.14 | +0.00 | 82.84 | -0.19 |
Modules without UTs
CFDP/Checksum/GTest, Drv/ByteStreamDriverModel, Drv/Interfaces, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports, Drv/Ports/DataTypes, FppTestProject/FppTest/interfaces, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/ports, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Cmd, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Interfaces, Fw/Obj, Fw/Port, Fw/Ports/CompletionStatus, Fw/Ports/Ready, Fw/Ports/Signal, Fw/Ports/SuccessCondition, Fw/Prm, Fw/SerializableFile/test/TestSerializable, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Cycle, Svc/DpPorts, Svc/Fatal, Svc/FatalHandler, Svc/FileDownlinkPorts, Svc/FprimeProtocol, Svc/Interfaces, Svc/PassiveConsoleTextLogger, Svc/Ping, Svc/PolyIf, Svc/Ports/CommsPorts, Svc/Ports/FilePorts, Svc/Ports/OsTimeEpoch, Svc/Ports/TlmPacketizerPorts, Svc/Ports/VersionPorts, Svc/Sched, Svc/Seq, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestChainedAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestTargetAutocoder, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent
|
@LeStarch Looks like some of the failures are due to missing zlib.h. Do you want me to find a solution to compiling on platforms without zlib? |
|
I'm a little confused by this error in the RHEL8 build Because both |
| // Component construction and destruction | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| DpCompressProc ::DpCompressProc(const char* const compName) : DpCompressProcComponentBase(compName) {} |
| // Component construction and destruction | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| DpZLibCompressor ::DpZLibCompressor(const char* const compName) : DpZLibCompressorComponentBase(compName) {} |
| ctx.comp.log_WARNING_LO_BufferTooBigForZLib(in_buffer.getSize(), ctx.compression_buffer.getSize(), | ||
| std::numeric_limits<uInt>::max()); | ||
| return CompressionAlgorithm::UNCOMPRESSED; | ||
| } |
| // The call to deflateEnd does not flush any additional output data | ||
| } | ||
|
|
||
| voidpf DpZLibCompressor::zlib_alloc_fn(voidpf opaque, uInt items, uInt size) { |
| // The call to deflateEnd does not flush any additional output data | ||
| } | ||
|
|
||
| voidpf DpZLibCompressor::zlib_alloc_fn(voidpf opaque, uInt items, uInt size) { |
| // Handler implementations for typed input ports | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| void DpCompressProc::serializeCompressionHeader(Fw::LinearBufferBase& serializer, |
| FW_ASSERT(ok == Fw::FW_SERIALIZE_OK, ok); | ||
| } | ||
|
|
||
| void DpCompressProc ::procRequest_handler(FwIndexType portNum, Fw::Buffer& fwBuffer) { |
| FW_ASSERT(ok == Fw::FW_SERIALIZE_OK, ok); | ||
| } | ||
|
|
||
| void DpCompressProc ::procRequest_handler(FwIndexType portNum, Fw::Buffer& fwBuffer) { |
| return alg; | ||
| } | ||
|
|
||
| CompressionAlgorithm DpZLibCompressor::zlibCompressionHelper(ZLibCtx& ctx, const Fw::Buffer& in_buffer) { |
| // The call to deflateEnd does not flush any additional output data | ||
| } | ||
|
|
||
| voidpf DpZLibCompressor::zlib_alloc_fn(voidpf opaque, uInt items, uInt size) { |
| DpZLibCompressor& comp; | ||
|
|
||
| ZLibCtx(DpZLibCompressor& c) | ||
| : compression_buffer(), zlib_alloc_buffer(), bump_allocator(0), zlib_stream(), comp(c) {} |
|
@kubiak-jpl this is a type of warning we're used to seeing on the RHEL8 compilers. |
I didn't realize that could happen. I made your requested change |
|
For the missing zlib.h - hopefully installing zlib/zlib-devel around here (in all 3 jobs) should fix it |
|
|
||
| instance dpCompressProc: Svc.DpCompressProc base id DataProductsConfig.BASE_ID + 0x04000 | ||
|
|
||
| instance dpZLibCompressor: Svc.DpZLibCompressor base id DataProductsConfig.BASE_ID + 0x05000 |
There was a problem hiding this comment.
CI is exposing that the zlib dependency introduced by the DipZLibCompressor component is not a standard library that's available on most distributions (ubuntu, RHEL8, macos runners all don't have it).
Adding this component to the DataProducts subtopology adds a new system dependency on zlib for systems that want to use this subtopology. I don't think this is desirable as a default option.
Likely the best way forward here is to make the inclusion of this component optional (build option, other subtopology, or something else). That would be awesome, but it's possibly more work than you want to be doing right now.
Given that the change is only adding port connections, a good-and-easy solution would be to add it in the topology of one of our references, but leave it out of the subtopology by default. https://github.com/nasa/fprime-examples/tree/pr-5235 would be a good place.
There may be other options too.
@kubiak-jpl thoughts? TL;DR multiple options, let me know what you prefer
There was a problem hiding this comment.
Another option would be to conditionally compile the component as a no-op if zlib isn't available. That would prevent breaking these other deployments, but it might add more confusion about whether compression is actually enabled or not.
Maybe there's a middle ground where the DpCompressProc component stays in the subtopology, but we omit the compressor. Then users could add compressors their system supports. Do subtopologies support exporting ports yet?
There was a problem hiding this comment.
Yeah conditionally compiling a feature with a no-op component doesn't really respect the intended component-based design.
And yes subtopologies can export ports, for the top-level topology to easily connect to: https://nasa.github.io/fpp/fpp-users-guide.html#Defining-Topologies_Topology-Ports
There was a problem hiding this comment.
How about this, I can update the DataProduct subtopology to export the DpWriter Proc port and I'll write a subtopology with DpCompressProc and DpZLibCompressor as an example of how to enable compression. Does that work? Would it make sense for the compression subtopology to exist in the fprime repo?
There was a problem hiding this comment.
Yes that's a great idea! Amazing.
If it were me I would keep the DpProc and DpZLib components in the same (optional) subtopology. That way to enable compression, all one needs to do is
import Subtopologies.DataProducts
+ import Subtopologies.DpCompression
[....]
+ DataProducts.procBufferSendOut[0] -> DpCompression.procRequest
Or is there a use-case for having a dpCompressProc component in the DataProducts subtopology even if it's not connected to anything?
There was a problem hiding this comment.
And this is actually a really nifty pattern for all DpProcessing systems...
There was a problem hiding this comment.
Sounds good. I'll also add some cmake logic to only expose the DpZLibCompressor component if zlib is found. Something with find_library
Change Description
This PR makes the following changes
memmoveinstead ofmemcpyserializeFrommethod calls which make the algorithm to re-write the data product cleaner. However, I can also revert this change and modify my code to usememmovedirectlyRationale
Implements lossless compressible data products seamlessly into the F Prime Data Product subsystem
Testing/Review Recommendations
The algorithm to modify the data product in place is non-trivial. I believe I have sufficiently thought through the state machine and have unit tests to cover all the cases. But it could use thorough review.
I had this integrated into the Ref deployment before that was moved.
Future Work
AI Usage (see policy)
N/A