Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for queue overflow hook methods #492

Merged
merged 11 commits into from
Aug 16, 2024
Merged

Conversation

LeStarch
Copy link
Collaborator

@LeStarch LeStarch commented Aug 12, 2024

When a port is set to 'drop' on overflow, there can be a memory leak should the port contain a resource that needed dedicated deallocation (e.g. Fw.Buffer). To fix this, we add a hook overflow option, which passes the port arguments to a function for handling (e.g. call to deallocate).

This PR contains the following items:

  • Spec updated
  • fpp-to-xml outputs 'hook' option
  • fpp-to-cpp component test updated (active, queued)
  • fpp-to-cpp templated test updated (active, queued)
  • fpp-to-cpp component generation
    • Internal ports (async)
    • Serial ports (async)
    • Data product special ports (async)
    • Typed ports (async)
    • Commands (async)
  • fpp-to-cpp template generation
    • Internal ports (async)
    • Serial ports (async)
    • Data product special ports (async)
    • Typed ports (async)
    • Commands (async)

F Prime integration work (to go):

  • FppTest examples
  • XML spec placeholder for 'hook'. Note: downgraded to 'drop' XML specification is deprecated.

@bocchino
Copy link
Collaborator

Looks good! Since we are no longer actively developing the XML model, I suggest we convert hook to drop before generating XML. We do something similar with data product special ports, converting them to ordinary ports before generating XML.

@LeStarch
Copy link
Collaborator Author

@bocchino did the downgrade from 'hook' to 'drop' in XML generation, along with a test that proves the feature.

Copy link
Contributor

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Peer review notes:

  • rename overflowHook to cmdOverflowHook for commands, following the cmdHandler pattern for commands vs handler for ports

@LeStarch LeStarch force-pushed the feat/overflow-hook branch from e7f172f to 59a0e6f Compare August 15, 2024 23:20
@LeStarch LeStarch requested a review from thomas-bc August 15, 2024 23:22
@LeStarch
Copy link
Collaborator Author

@bocchino I have fixed all review comments, please let me know if anything else is needed.

Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! This is a good addition to the modeling and code gen.

@bocchino
Copy link
Collaborator

@thomas-bc This looks good to me. Once you review and approve, I will merge!

Copy link
Contributor

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 🇮🇹

@bocchino bocchino merged commit f5eae43 into main Aug 16, 2024
11 checks passed
@bocchino bocchino deleted the feat/overflow-hook branch August 16, 2024 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants