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

Remove vss2dbc mapping for sensor #24

Merged
merged 1 commit into from
May 20, 2024

Conversation

erikbosch
Copy link
Contributor

Related to #23

Current CAN provider implementation always subscribe to target value for "vss2dbc". That does not really make sense for sensors, as they do not have target values. Remove it from example mapping and give a warning

@erikbosch erikbosch marked this pull request as ready for review May 16, 2024 13:04
if node["type"] != "actuator":
# vss2dbc is handled by subscription to target value, so only makes sense
# for actuators
log.warning("VSS signal %s is not an actuator, vss2dbc not relevant to use", expanded_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what this code does or want to say.

"relevant to use"

The menaing is "vss2dbc only supported for actuators", right? However if it is right, this code does nothing, it just prints something, shouldn"t it ignore such amppings then?

So I not fully get what happens actually if there is a mapping of vss2dbc for a sensor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Below is the code that manages subscription. As of today I do not think that you get an error if trying to subscribe to target value for a sensor/attribute. (Alternatively we discard any error message we receive, but I do not think that is the case.)

This code gives a warning if using vss2dbc on a sensor or attribute, but it does not ignore the vss2dbc statement or give an error. So it is just a hint to the user that they maybe should refactor their mapping file. We could be more intrusive:

  • We could ignore the vss2dbc after giving warning. Practical behavior would be the same as we anyway do not get any updates for sensors/attributes. The only difference would be if there are errors inside the vss2dbc statement, today that would result in errors, if we ignore it they would be no errors either.
  • We could give an error and exit.

And we could rephrase the warning but keep the rest intact if we would like. Any opinion?

async def subscribe(self, vss_names: List[str], callback):
        """Create a subscription and invoke the callback when data received."""
        entries: List[SubscribeEntry] = []
        for name in vss_names:
            # Always subscribe to target
            subscribe_entry = SubscribeEntry(name, View.FIELDS, [Field.ACTUATOR_TARGET])
            log.info("Subscribe entry: %s", subscribe_entry)
            entries.append(subscribe_entry)

        # If there is a path VSSClient will request a secure connection
        if self._tls and self._root_ca_path:
            root_path: Optional[Path] = Path(self._root_ca_path)
        else:
            root_path = None

        async with VSSClient(self._ip, self._port, token=self._token,
                             root_certificates=root_path, tls_server_name=self._tls_server_name) as client:
            async for updates in client.subscribe(entries=entries):
                log.debug(f"Received update of length {len(updates)}")
                await callback(updates)

@SebastianSchildt
Copy link
Contributor

I think the "best way" is printing a waring and actually dropping the mapping.
The API not giving an error when subscribing target for a sensor could be seen as an error, and f that ever changes might break tis code. So I think dropping such mappings is better.

On the other hand, I don"t want to overcomplicate this PR, so I can also follow any of the other suggestions.
(Failing with exit might also be good, as it at least points to a problem early)

@erikbosch
Copy link
Contributor Author

Solution changed to exit if using vss2dbc on something not an actuator

Related to eclipse-kuksa#23

Current CAN provider implementation always subscribe to target value for "vss2dbc".
That does not really make sense for sensors, as they do not have target values
Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

lgtm 🦘

@SebastianSchildt SebastianSchildt merged commit b5699d6 into eclipse-kuksa:main May 20, 2024
7 checks passed
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