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

Case-specific identifier names? #3164

Open
zimri-leisher opened this issue Jan 28, 2025 · 5 comments
Open

Case-specific identifier names? #3164

zimri-leisher opened this issue Jan 28, 2025 · 5 comments
Labels

Comments

@zimri-leisher
Copy link
Collaborator

zimri-leisher commented Jan 28, 2025

F´ Version 3.5.1
Affected Component fprime identifier specification

Problem Description

In an admittedly awful situation, if I wanted to name one parameter param1 and another Param1, the FPrime specification does not mention whether this is valid, as it does not mention whether capitalization has syntactic meaning for identifiers. Right now, it seems like this would work, if it were not for the fact that the auto-generated PRM_SET/SAVE commands automatically capitalize the parameter names. This means that two parameters with a name which differs only by capitalization conflict because their SET/SAVE cmds have the same name.

Likewise, similar thing happens for telemetry. It is valid FPP, but the compilation step crashes because there are two enum consts named CHANNELID_XXXX, where XXXX is the name of the telemetry channel being duplicated.

Context / Environment

Execute fprime-util version-check and share the output.

Operating System: Linux
CPU Architecture: x86_64
Platform: Linux-5.15.133.1-microsoft-standard-WSL2-x86_64-with-glibc2.35
Python version: 3.10.12
CMake version: 3.22.1
Pip version: 24.2
Pip packages:
    fprime-tools==3.5.1
    fprime-gds==0.1.dev1222+gb4c4122.d20250128
    fprime-fpp-*==2.3.0a1
Project submodules:

How to Reproduce

  1. Go to Ref
  2. Create a copy of a channel or param with a name which differs only by case
  3. Observe error messages

Expected Behavior

Maybe this should be caught at the FPP level?
This is certainly a minor problem, but it is worth mentioning.

@LeStarch
Copy link
Collaborator

Agreed. Thanks for posting this. For now the recommendation would be to use case-insensitive uniqueness in name. However, it is worth taking a look.

@bocchino
Copy link
Collaborator

I don't think this is a bug. It is a known behavior, which we can consider revising. Should we move this issue to a discussion?

My personal view is that we should do one of two things:

  1. Accept the current behavior. It works OK except in pathological cases that you shouldn't provoke anyway.
  2. Revise the code gen strategy so that all names in the C++ code and the dictionary exactly match the spelling of the corresponding names in the model. If the model has a parameter FooBar, then the parameter in the dictionary should be FooBar, the set command should be SET_PRM_FooBar, the C++ enum constant for the opcode should be OPCODE_FooBar, etc.

In case 2, if we want parameters in the dictionary to have the form FOOBAR or FOO_BAR, then we could require that form in the model. For example, we could disallow a parameter name FooBar in the model. In this approach, we would just exclude lower-case characters from identifiers in certain names.

A third option is to specify and check a bunch of rules about what gets transformed to what in the generated code. For example, you can write FooBar in the model and it's transformed to FOOBAR in the dictionary; but then you can't use FOOBAR for a different symbol of the same kind in the model. To me this approach seems less than ideal, for several reasons:

  1. It makes the specification and analysis of the modeling language depend on the details of code generation. This seems backwards. The modeling language should impose the rules, and the code gen should be straightforward.
  2. It seems "wrong" in some sense: if the target system (C++, GDS) ultimately requires that the name is FOOBAR, why are we allowing a different name in the model and then rewriting it? Why not require the correct name to begin with?
  3. When upper and lower case are mixed in the model, the auto-transformation generates ugly names. For example, if you require a user to write in snake case, the user might write a name like ATTITUDE_CONTROL. But if you allow the user to write AttitudeControl and transform it in the obvious way, you get ATTITUDECONTROL, which is ugly.

@bocchino
Copy link
Collaborator

Note also that the current approach (allow FooBar and transform to FOOBAR) was inherited from the XML autocoders, which we are phasing out.

@zimri-leisher
Copy link
Collaborator Author

I was thinking that this could be fixed by checking FPP identifier uniqueness case-insensitively, and adding such a qualifier to the FPP Language Spec/User's guide. That way, the behavior which is already implicitly not allowed becomes explicitly not allowed, and we don't have to worry about supporting it. I think in general I'm working under the assumption that all valid FPP code should have a valid CPP equivalent, but this is probably naive. I understand that you don't want design decisions about FPP to be made with reference to the CPP autocode, that makes sense to me.

@LeStarch
Copy link
Collaborator

LeStarch commented Feb 3, 2025

Current recommendation:

  • Names should not be transformed by FPP at all. Users desiring UPPER_CASE_NAMES should use UPPER_CASE_PARAMETER_NAMES

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants