fprime icon indicating copy to clipboard operation
fprime copied to clipboard

Add Fw::ObjectName to hold Fw::ObjBase name

Open thomas-bc opened this issue 1 year ago • 2 comments

Related Issue(s) #2493
Has Unit Tests (y/n) y
Documentation Included (y/n) n


Change Description

Implements Fw::ObjectName to hold names of ObjBase objects instead of raw char arrays.

I tried to keep the changes to a minimum, so I'm using .toChar() in most places. Let me know if there's other patterns you think I should add here, or any other desired changes.

Associated PR to FPP: https://github.com/fprime-community/fpp/pull/385

Rationale

See #2493

thomas-bc avatar Jan 23 '24 02:01 thomas-bc

This looks great! I suggest we do the following:

  • [x] Merge https://github.com/nasa/fprime/pull/2485. This will bring fpp/main into sync with fprime/devel.
  • [x] Merge https://github.com/fprime-community/fpp/pull/385.
  • [x] Make a new pre-release of FPP.
  • [x] Update this PR to refer to the FPP pre-release and pass CI.
  • [ ] Merge this PR.

bocchino avatar Jan 23 '24 22:01 bocchino

Added the recommended changes.

I wanted to apply them to the other Fw::*String types to resolve the same warnings in those classes, but that broke some implicit conversions in, at least, some UTs (e.g. LoggerMain.cpp) Didn't want to go out of scope for this PR or risk some downstream breakages so I didn't touch anything, but might be worth fixing in the future.

thomas-bc avatar Jan 24 '24 01:01 thomas-bc

I am working on integrating this PR with FPP. It's looking good so far. Here is one issue I see: FW_OBJECT_NAME_MAX_SIZE is technically not a correct name. It should be FW_OBJECT_NAME_BUFFER_SIZE or something like that. The max string length is the buffer size minus one, because of the terminating zero. Compare FW_OBJ_TO_STRING_BUFFER_SIZE, which gets this right.

bocchino avatar Feb 26 '24 18:02 bocchino

@bocchino this also applies to FW_QUEUE_NAME_MAX_SIZE, FW_TASK_NAME_MAX_SIZE, and probably others. Do you want me to change these in this PR, or should we capture this in a new issue and get back to it in a different PR?

thomas-bc avatar Feb 26 '24 22:02 thomas-bc

Since this is a breaking change for projects that define their own config/ folder, I would suggest we do the name changes all at once in a different PR, and keep the scope of this PR as is

thomas-bc avatar Feb 26 '24 22:02 thomas-bc

I agree, let's defer this to a separate issue.

bocchino avatar Feb 26 '24 22:02 bocchino