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

Update the ISO 15118-2 DC demo with -2 message logs #3

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

drmrd
Copy link
Contributor

@drmrd drmrd commented Nov 17, 2023

This PR aligns the demo with what we've been showing during industry outreach events. Changes to the ISO 15118-2 DC demo's Node-Red flows result in all ISO 15118-2-related messages being displayed in a table. The actual request and response messages remain EXI-encoded. A V2G decoder (such as this one based on RISE-V2G) can then be used to decode these messages.

Future improvements could include incorporating a Node-based V2G decoder into the dashboard or to log un-encoded V2G messages more directly. The message types that are published during the demo's 15118-2 request-response message sequence(s) are displayed above the table in real-time, as well.

@drmrd drmrd closed this Nov 17, 2023
@drmrd drmrd deleted the add-15118-dash-2-logs-to-dc-demo branch November 17, 2023 15:32
@drmrd drmrd restored the add-15118-dash-2-logs-to-dc-demo branch November 17, 2023 15:33
@drmrd drmrd reopened this Nov 17, 2023
@drmrd drmrd marked this pull request as draft November 17, 2023 15:36
@drmrd drmrd marked this pull request as ready for review November 17, 2023 15:37
@drmrd drmrd marked this pull request as draft November 17, 2023 15:37
@drmrd drmrd changed the title Display ISO 15118-2 messages in DC demo Update the ISO 15118-2 DC demo with -2 message logs Nov 17, 2023
Copy link
Collaborator

@shankari shankari left a comment

Choose a reason for hiding this comment

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

LGTM as a temporary fix. However, note that the config here is one that I copied from everest-core to get the demo to work. We should consider submitting the new config to everest-core or wherever we end up with config examples.

@hikinggrass, this is a change to the node-red config in the demo repo. The original file is copied over from everest-core, where there are a lot of sample configs. I anticipate that the set of sample configs will grow significantly, as we do more end-to-end testing with various configurations.

We might want to consider where they should live in the long term and how to make it easy for others to discover them.

I have also filed an internal issue to track the potential improvements (toggle on/off, toggle encoded/decoded)

Copy link
Collaborator

@shankari shankari left a comment

Choose a reason for hiding this comment

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

Since there are no packages in everest, and there will not be any until the build process is finalized
The only currently built packages are here:
https://github.com/orgs/US-JOET/packages

docker-compose.iso15118-dc.yml Outdated Show resolved Hide resolved
docker-compose.iso15118-dc.yml Outdated Show resolved Hide resolved
docker-compose.iso15118-dc.yml Outdated Show resolved Hide resolved
@drmrd drmrd marked this pull request as ready for review November 20, 2023 16:55
@shankari
Copy link
Collaborator

@drmrd are you sure that this works as checked in? I tried testing it and I wasn't able to launch the node-red server or the UI
Screen Shot 2023-11-20 at 9 52 44 PM

Logs from the launch

  • Checking out this branch
$ git checkout --track origin/add-15118-dash-2-logs-to-dc-demo
  • Launching the docker compose
$ docker compose -f docker-compose.iso15118-dc.yml up
[+] Building 0.0s (0/0)                                                             docker:desktop-linux
[+] Running 3/0
 ✔ Container everest-demo-mqtt-server-1  Created                                                    0.0s
 ✔ Container everest-demo-node-red-1     Created                                                    0.0s
 ✔ Container everest-demo-manager-1      Created                                                    0.0s
Attaching to everest-demo-manager-1, everest-demo-mqtt-server-1, everest-demo-node-red-1
  • Everest has started up
everest-demo-manager-1      | 2023-11-21 05:49:34.890996 [INFO] manager          :: 🚙🚙🚙 All modules are initialized. EVerest up and running [2087ms] 🚙🚙🚙
everest-demo-manager-1      | 2023-11-21 05:49:37.582542 [INFO] evse_manager:Ev  :: 🌀🌀🌀 Ready to start charging 🌀🌀🌀
  • But we get MQTT errrors while trying to connect node-red
everest-demo-mqtt-server-1  | 1700545789: New connection from 192.168.65.1:38789 on port 1883.
everest-demo-mqtt-server-1  | 1700545789: Client <unknown> disconnected due to protocol error.
everest-demo-mqtt-server-1  | 1700545789: New connection from 192.168.65.1:38790 on port 1883.
everest-demo-mqtt-server-1  | 1700545789: Client <unknown> disconnected due to protocol error.
everest-demo-mqtt-server-1  | 1700545789: New connection from 192.168.65.1:38791 on port 1883.
everest-demo-mqtt-server-1  | 1700545789: Client <unknown> disconnected due to protocol error.

@couryrr-afs
Copy link
Contributor

@shankari I think you have a typo in the browser. You will want to access localhost:1880/ui to access the node-red ui.

@shankari
Copy link
Collaborator

shankari commented Nov 21, 2023

Ooops! This is why I shouldn't test at 10pm.

This does launch now, but it doesn't include the new logging functionality, since the image has not been updated.
Since this is a common theme, and @drmrd is out, I have created a new branch (build_pkg_task_1_features) - you can push all these changes to that branch as well to build an image that includes them, and then update all these PRs to use the image tag.

@couryrr-afs
Copy link
Contributor

@shankari if I am reading this correctly the creation of images looks to be postponed until the PR is merged. This is so that there is not functionality within the running container that relies on the code in the PR. To test a PR's changes locally you can build that image yourself when pulling down the code. I am leaning toward adding a latest tag to the workflow that would pin the latest PR that is merged. Then in all of the docker-compose.yml we can just reference :latest for now. Then we do not need to go through an extra branch.

@shankari
Copy link
Collaborator

shankari commented Nov 21, 2023

@couryrr-afs as you can see from the action and the issue, the current flow is:

  • you push to 'build_pkg_**' (either through PR or direct push) currently in the US-JOET org
  • images are built and uploaded automatically to the US-JOET org
  • you change the docker-compose to use the new image and re-push the PR

Yes, this is clunky. We should fix it even in the short term

@drmrd
Copy link
Contributor Author

drmrd commented Nov 29, 2023

Closing this PR in favor of US-JOET#3 to accommodate existing Docker build process.

@drmrd drmrd closed this Nov 29, 2023
@drmrd
Copy link
Contributor Author

drmrd commented Nov 29, 2023

Re-opening for the time being, with the intent being to finish a review after we update the CI in #10.

@shankari
Copy link
Collaborator

shankari commented Dec 5, 2023

@drmrd if you can resolve the merge conflict and bump up the version, this one has been approved and is good to go.

@drmrd drmrd force-pushed the add-15118-dash-2-logs-to-dc-demo branch from 493348c to 994ba30 Compare December 6, 2023 01:02
@drmrd drmrd force-pushed the add-15118-dash-2-logs-to-dc-demo branch from 994ba30 to 2826f23 Compare December 6, 2023 01:14
@drmrd drmrd requested a review from shankari December 6, 2023 01:14
@drmrd
Copy link
Contributor Author

drmrd commented Dec 6, 2023

@shankari: I believe this PR is updated and ready to go.

@shankari shankari merged commit 0c0d8e2 into EVerest:main Dec 6, 2023
4 checks passed
@shankari shankari deleted the add-15118-dash-2-logs-to-dc-demo branch December 6, 2023 03:02
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.

3 participants