[RSDK-447] Opt-in to implicit dependencies for all components #1488
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| } | ||
| if cfg.Power == "" { | ||
| return utils.NewConfigValidationFieldRequiredError(path, "power") | ||
| return nil, utils.NewConfigValidationFieldRequiredError(path, "power") | ||
| } | ||
|
|
||
| if cfg.AnalogReader != "psi" { |
There was a problem hiding this comment.
(unrelated to this PR) That's a very strange requirement. If this doesn't make sense to you either, can you please open a ticket to remove this requirement?
There was a problem hiding this comment.
I am not familiar with the softrobotic gripper, looks like its supposed to be a pin so make sense its required
There was a problem hiding this comment.
This is because of the ADC magic we do to make this gripper work with a pi. We can remove all these requirements and return warnings if we ever want to break this code.
| } | ||
|
|
||
| // Validate ensures all parts of the config are valid. | ||
| func (config *Config) Validate(path string) ([]string, error) { |
There was a problem hiding this comment.
Why no buttons and axes validation? Should they be marked optional?
There was a problem hiding this comment.
Honestly would have to ask @Otterverse. I don't think anyone in nyc has used a gpio input controller.
There was a problem hiding this comment.
Since he's on PTO, I looked at the file, I suggest omitting them both as empty by default, and only returning an error or warning if both Buttons and Axes are empty. There can be a use case where you use the joystick or the buttons only. @npmenard
| @@ -42,6 +42,28 @@ type TMC5072Config struct { | |||
| HoldDelay int32 `json:"hold_delay,omitempty"` // 0=instant powerdown, 1-15=delay * 2^18 clocks, 6 default | |||
| } | |||
|
|
|||
| // Validate ensures all parts of the config are valid. | |||
| func (config *TMC5072Config) Validate(path string) ([]string, error) { | |||
There was a problem hiding this comment.
(I don't know much about this component) Are all of these really required? Should pins be validated too?
There was a problem hiding this comment.
@Rob1in Can you help us out here?
| @@ -30,32 +30,33 @@ type AttrConfig struct { | |||
| SPI string `json:"spi"` | |||
| Speed *int `json:"spi_baud_rate"` | |||
| Pfreq *int `json:"polling_freq_hz"` | |||
| CSPin string `json:"cs_pin"` | |||
| CSPin string `json:"chip_select_pin"` | |||
There was a problem hiding this comment.
this is a breaking change, let's do this separately and bundle with other breaking changes
There was a problem hiding this comment.
I don't think anyone is using a vectornav except Nicolas. Might be okay?
There was a problem hiding this comment.
Yes i am the only one so i vote we should do it :)
randhid
requested changes
Oct 18, 2022
| } | ||
|
|
||
| // Validate ensures all parts of the config are valid. | ||
| func (config *Config) Validate(path string) ([]string, error) { |
There was a problem hiding this comment.
Honestly would have to ask @Otterverse. I don't think anyone in nyc has used a gpio input controller.
| @@ -42,6 +42,28 @@ type TMC5072Config struct { | |||
| HoldDelay int32 `json:"hold_delay,omitempty"` // 0=instant powerdown, 1-15=delay * 2^18 clocks, 6 default | |||
| } | |||
|
|
|||
| // Validate ensures all parts of the config are valid. | |||
| func (config *TMC5072Config) Validate(path string) ([]string, error) { | |||
There was a problem hiding this comment.
@Rob1in Can you help us out here?
| @@ -30,32 +30,33 @@ type AttrConfig struct { | |||
| SPI string `json:"spi"` | |||
| Speed *int `json:"spi_baud_rate"` | |||
| Pfreq *int `json:"polling_freq_hz"` | |||
| CSPin string `json:"cs_pin"` | |||
| CSPin string `json:"chip_select_pin"` | |||
There was a problem hiding this comment.
I don't think anyone is using a vectornav except Nicolas. Might be okay?
This comment was marked as off-topic.
This comment was marked as off-topic.
randhid
requested changes
Oct 19, 2022
randhid
approved these changes
Oct 24, 2022
npmenard
added
the
safe to test
viambot
removed
the
safe to test
edaniels
added
the
safe to test
viambot
removed
the
safe to test
npmenard
added
the
safe to test
viambot
removed
the
safe to test
npmenard
added
the
safe to test
viambot
removed
the
safe to test
npmenard
added
the
safe to test
viambot
removed
the
safe to test
npmenard
added
the
safe to test
|
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also address RSDK-444