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

Rename actuator classes #87

Merged
merged 7 commits into from
Apr 5, 2023
Merged

Conversation

alex-brinkman
Copy link
Contributor

Renames:

  • EgdActuator -> GoldActuator
  • EpdActuator -> PlatinumActuator
    to promote readability after consensus was reached by the development team.
    Also Update the Documentation for each of these devices.

I've added Actuator class parsing support to backwards compatibility until we release v1.0.0 eventually.

[ WARN  ] [1680539897.571716] (manager.cc:644) Starting in v0.12.0, Platinum device support has been added to Fastcat!                                                                               
[ WARN  ] [1680539897.571719] (manager.cc:645) Therefore the 'Actuator' class has been renamed to the 'GoldActuator' to make room for the new 'PlatinumActuator' device                              
[ WARN  ] [1680539897.571721] (manager.cc:646) Support for 'Actuator' is eventually going to be dropped in v1.0.0 so update your YAML now to prevent testbed downtime   

@alex-brinkman alex-brinkman requested a review from d-loret April 3, 2023 17:05
@alex-brinkman
Copy link
Contributor Author

With this merge complete, I'll mint v0.12.0 and unveil the platinum driver to the world!

@d-loret
Copy link
Contributor

d-loret commented Apr 3, 2023

@alex-brinkman, we might still want to make it a major release since, even not modifying Actuator, it will break backward compatibility with client applications simply because of the splitting of state.

@alex-brinkman
Copy link
Contributor Author

So prior to v1.0.0, I'm falling back to this rule:
Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.
https://semver.org/#spec-item-4
For V0.y.z development, we treat Major releases as Minor releases and both Minor/Patch releases as a Patch release. i.e. everything shifts over on number.

I've found that as soon as we mint v1.0.0 after a big development thrust, we tend to need to jump to v2.0.0 or even v3.0.0 really quickly, so I'd prefer to let the dust settle on these change for a few months and target the v1.0.0 release for this summer, aligning with other application schedules.

Copy link
Contributor

@d-loret d-loret left a comment

Choose a reason for hiding this comment

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

Aside from the comments in the Egd's documentation and minor details I pointed out, my only major observation is that we are allowing the use of Actuator in bus topologies even though we need to update bus topologies right now because of the renaming of egd_* parameters. I think it would be simpler to update everything at the same time.

As for the naming, I don't remember what was the consensus for the name of the base class, wasn't it ActuatorBase or something along those lines? I don't have a preference on this one, but I was left wondering.

doc/fastcat_device_config_parameters.md Outdated Show resolved Hide resolved
doc/fastcat_device_config_parameters.md Outdated Show resolved Hide resolved
doc/fastcat_device_config_parameters.md Show resolved Hide resolved
doc/fastcat_device_config_parameters.md Outdated Show resolved Hide resolved
doc/fastcat_device_config_parameters.md Show resolved Hide resolved
src/jsd/platinum_actuator_offline.h Outdated Show resolved Hide resolved
src/manager.cc Outdated Show resolved Hide resolved
@d-loret
Copy link
Contributor

d-loret commented Apr 4, 2023

So prior to v1.0.0, I'm falling back to this rule
Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Hehe, we should be in this rule indefinitely.

My comment stem from here.

@alex-brinkman
Copy link
Contributor Author

I'd rather not have the Actuator Class be backwards compatible so I'm going to go ahead and remove the special handling of this string.

@alex-brinkman
Copy link
Contributor Author

If a topology contains Actuator it now is rejected but with helpful deprecated info still:

[ WARN  ] [1680729778.168764] (fastcat/src/manager.cc:657) Starting in v0.12.0, Platinum device support has been added to Fastcat!
[ WARN  ] [1680729778.168767] (fastcat/src/manager.cc:658) Therefore the 'Actuator' class has been renamed to the 'GoldActuator' to make room for the new 'PlatinumActuator' device
[ ERROR ] [1680729778.168770] (fastcat/src/manager.cc:659) Update your topology for all 'Actuator' entries
[ ERROR ] [1680729778.168773] (fastcat/src/manager.cc:140) Failed to configure Offline bus
[SUCCESS] [1680729778.168776] (fastcat/test/test_config.cc:23) File successfully parsed!
[SUCCESS] [1680729778.168779] (fastcat/src/manager.cc:78) Freed JSD memory

@alex-brinkman
Copy link
Contributor Author

As for the naming, I don't remember what was the consensus for the name of the base class, wasn't it ActuatorBase or something along those lines? I don't have a preference on this one, but I was left wondering.

The internet does not show consensus here so let's leave it to reduce code change.

@alex-brinkman alex-brinkman merged commit 90129a4 into master Apr 5, 2023
@alex-brinkman alex-brinkman deleted the apb-rename-actuator-classes branch April 5, 2023 21:50
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