Skip to content

Comments

Feature/typed entity parameters#36

Closed
ineeve wants to merge 3 commits intoLSTS:feature/unifyfrom
ineeve:feature/typed-entity-parameters
Closed

Feature/typed entity parameters#36
ineeve wants to merge 3 commits intoLSTS:feature/unifyfrom
ineeve:feature/typed-entity-parameters

Conversation

@ineeve
Copy link

@ineeve ineeve commented May 11, 2023

Changes:

  1. Adds values_list and values_if_list fields to the TypedEntityParameter message.
  2. Creates a new ValuesIf message (id 2014) which is used in the values_if_list field of the TypedEntityParameter

@paulosousadias
Copy link
Member

paulosousadias commented May 15, 2023

about the PR of the params.
Some small things to adjust:

  • I'll just change the order of the enums for consistency with the others, it's rather like this: value id="0" name="User" abbrev="USER" />
  • I would like to change the name of the message TypedEntityParameters to QueryTypedEntityParameters (they are almost the same names, but one of them is plural, so it is clearer and more in line with what has already been done for other msgs)
  • should actually be "Parameter Name" as the entity name is the section
  • I would like the 3 msgs to be together in terms of ids, it may seem like a detail, but things are starting to be scattered and even in the generated documentation scattered, or not followed in terms of use
  • The default value for a string may have some edgecases in the sense that the string value of "" (empty string) may actually be empty or not default (for numbers it's easy). But since I didn't implement it, I'm not seeing a problem for now.
  • is list_min/max_size really necessary to be uint32? Isn't it too big for what you're usually going to use? (it takes up space when sending messages that in most cases are not listed), the question is whether 255 values or 65535? Maybe I don't see much use, what do you think?

@ineeve
Copy link
Author

ineeve commented May 16, 2023

Thank you for your recommendations @paulosousadias

  • I updated the order, but the IMC.xml file is not consistent on this.
  • Updated to QueryTypedEntityParameters
  • That's correct, it should be 'Parameter Name'. It is now fixed.
  • I grouped them into the ids 2016, 2017 and 2018. However, now there are "holes" in 2008, 2009, and 2014;
  • Regarding the default value of the strings we haven't noticed any issue yet, but I'll keep an eye.
  • I checked our dune version and our list_min/max_size fit always into uint8_t, so I think it will be fine to use uint8_t;

@ineeve
Copy link
Author

ineeve commented Aug 2, 2023

Replaced by #37

@ineeve ineeve closed this Aug 2, 2023
@ineeve ineeve deleted the feature/typed-entity-parameters branch August 2, 2023 14:01
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.

2 participants