-
Notifications
You must be signed in to change notification settings - Fork 22
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 README.md #73
Update README.md #73
Conversation
Updated the EVerest Demo README to improve readability, set-up instructions, and include mermaid diagrams of the various demonstrations that are included in the repository. Signed-off-by: nserway <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nserway as I have indicated earlier, I would like to see some additional discussion of the rationale behind the PR in the description.
What are the high level changes?
What was the related design discussion?
Why are the changes set up this way?
This is particularly important in this case because the main goal of the rewrite was to redesign the user interface and provide a template for future changes to follow.
Please also make sure to spellcheck, I am not sure we got all the errors.
README.md
Outdated
| **One EV ↔ EVSE (AC Simulation)** | Simple AC charging session with one EV connecting to one Charger (EVSE) | [One EV ↔ EVSE (AC Simulation) Diagram](#One-EV-to-EVSE-(AC-Simulation)) | | ||
| **One EV ↔ EVSE (ISO 15118-2 DC)** | ISO 15118-2 compliant charging session with one EV connecting to one EVSE | [One EV ↔ EVSE (ISO 15118-2 DC) Diagram](#One-EV-to-EVSE-(ISO-15118-2-DC))| | ||
| **Two EV ↔ EVSE** | Two EVSE connector points showcasing EVerests ability to work with a CSMS in a multi-station context | [Two EV ↔ EVSE Diagram](#Two-EV-to-EVSE) | | ||
| **E2E Automated Tests** | Performs an automated test of a full charging session| N/A | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this N/A?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did not create a mermaid diagram for the E2E tests. Since there isn't a UI associated with the demonstration (or if there is, the demo is not functioning properly) it was difficult to visualize what a diagram would look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mermaid diagram is not related to the UI. The mermaid diagram is set up to understand how the various software modules communicate with each other, and the E2E test clearly has modules working together for the testing.
README.md
Outdated
|
||
1. Install docker with the following link [Get Docker](https://docs.docker.com/get-docker/) | ||
|
||
- Note: When runing the demonstrations, use the Docker desktop terminal for the best results. However, demonstration commands can be posted directly into your machine's terminal if running on Windows or Linux systems. |
There was a problem hiding this comment.
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 you mean by this.
- The Docker desktop terminal can be used to interact with running containers. It cannot be used to launch containers from a script.
- And I am not sure why the Docker desktop terminal would give better results than (say)
docker exec
- Spelling mistake "runing"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This note was spun out of a conversation with Katie about the additional functionality of Linux systems (i.e., running a demo script directly in a Linux machine's terminal) I will remove this note for clarity as I do not think it adds significant value to the overall README + created confusion on your review.
- See above.
- Just ran a spell check across the whole document + updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am arguing for not using the docker desktop terminal and instead, using the command line.
README.md
Outdated
|
||
- Note: The terminal should return "Docker version x.x.x". | ||
|
||
3. Open the Docker desktop application and navigate to the terminal at the bottom of the screen. |
There was a problem hiding this comment.
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 you mean by this.
Here's the full docker desktop window on my mac. I don't see any terminal at the bottom of the screen.
Also, note that, on linux, docker does not come with the desktop UI by default.
https://docs.docker.com/desktop/install/linux/ ("What is the difference between Docker Desktop for Linux and Docker Engine?") Docker Desktop for Linux requires a license for commercial use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nserway "works for me" is not a good basis for demo instructions. I assume you are using Windows for this. Even if it works on Windows, that is 1/3 of the common OSes.
You should retain the instructions that work for the 2/3 case, which is to run it from the terminal.
README.md
Outdated
EV->>EVSE: Plug-in | ||
EVSE-->>EV: Proximity Check (PP Signal) | ||
EV->>EVSE: Detect Proximity | ||
EVSE-->>EV: Control Pilot Signal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that the simulator is actually sending these values? How have you verified this?
![image](https://everest.github.io/nightly/_images/quick-start-high-level-1.png) | ||
participant EV as Electric Vehicle (EV) | ||
participant EVSE as Electric Vehicle Supply Equipment (EVSE) | ||
Note over EV,EVSE: Connection Establishment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to cover CP and PP? If so, you need to define what this means somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the PP and CP checks are a part of this charging session as defined in the ISO 15118-2 documentation. https://www.typhoon-hil.com/documentation/typhoon-hil-software-manual/References/iso15118_protocol.html. We will make changes in the README to cite this documentation.
Note over EVSE: EVSE starts charging session | ||
EVSE ->> CS: StartTransaction.req (Session ID, EV info) | ||
CS -->> EVSE: StartTransaction.conf (Accepted) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is simpified at best and incorrect at worst. Is there a reason that we are not showing the full flow? What is the goal of the diagram, and what is the level of granularity that you are aiming for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal of the diagram was to provide a high level (non-granular) illustration of what is being demonstrated across the Demos. I was aiming for a very simplified version of the charging session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want a high-level description, I would avoid using specific ISO names (StartTransactionReq
) because that implies that those are the messages getting sent. You should instead use a high-level text description.
|
||
## Notes for Demo Contributors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed? From our discussion, we were expecting that people who contribute new functionality will create new demos and update the documentation. And one of the main goals of your improvements was to create and document the related process, which @the-bay-kay and I would use to make our upcoming changes. Where is that process?
What are the high level changes? The team re-designed the EVerest Demo README to improve: readability, flow of instructions, and showcasing demonstration capabilities. |
Running Spell Check Signed-off-by: nserway <[email protected]>
Remove note in # Install and Set-up section Signed-off-by: nserway <[email protected]>
Fix "safelinks" in instructions Signed-off-by: nserway <[email protected]>
remove 1.6 functionality Signed-off-by: nserway <[email protected]>
@nserway can you please have the tech team review this before sending it back to me for review? Some of your comments have fairly simple technical solutions, and it would be the most effective use of my time to have them addressed first. |
Adding the process for updating documentation if a community member wanted to contribute a demo to the repo Signed-off-by: nserway <[email protected]>
Added terminal instructions + reference material Signed-off-by: nserway <[email protected]>
Updated diagrams Signed-off-by: nserway <[email protected]>
One of the goals of open-source software is transparency into the decision making process. So we don't just want to have a work product, we want to document the discussions and deliberation that went into the work product. So yes, we want to document what were the user types you considered, what assumptions you made for them... |
Codacy Changes Signed-off-by: nserway <[email protected]>
Added e2e diagram and reference material Signed-off-by: nserway <[email protected]>
Signed-off-by: K. Shankari <[email protected]>
Signed-off-by: K. Shankari <[email protected]>
This is not great, but it is an improvement over what we had before, so I am going ahead and checking this in now. |
Updated the EVerest Demo README to improve readability, set-up instructions, and include mermaid diagrams of the various demonstrations that are included in the repository.
@shankari, per your most recent comments, here are our additions + changes:
.img
but adding the file to this PR so the new PR owner can add it to the.img
folder