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

Add sd/sc to allow setting date/time in Click PLC #43

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ssweber
Copy link

@ssweber ssweber commented Nov 21, 2024

Hello, Thanks for the library!

I wrote a short script to sync all my clicks on the network with my computer's clock. sync_clickplc_datetime.py I then realized that writing SD, and read/writing SC wasn't added yet. I added necessary changes / modeling them after the C and DS corresponding functions. I tested my script, and it did set correctly!

Let me know if it looks OK. Thanks

Makes it more clear that there are a total of X bits, not an additional X bits + the starting one
Modeled after _set_ds

Restricted it to only the writable SD registers
Modeled after C coils.

Restricted writing to only writable SC addresses
@ssweber
Copy link
Author

ssweber commented Nov 21, 2024

I'll look into the test_driver.py and see about following up with additional tests for this.

I didn't run ruff format, because it'd be harder to see what changes I made. If you're open for further contributions, maybe you could run 'ruff format . --config "format.quote-style = 'single'" (or omit config style if you like double-quotes).

@ssweber
Copy link
Author

ssweber commented Nov 21, 2024

Also I’ll send a commit over to update docs / user-facing on which sc/sd are writable

@alexrudd2
Copy link
Owner

Hello @ssweber

With the Thanksgiving holiday, I'm only seeing this now. Thanks so much for the PR! An initial review looks good, but I want to double check the register value changes.

I'm traveling next week but will be able to check this against real hardware 12/9. Also, I have a local work-in-progress branch adding SD, so I'll cross-check our implementations.

I didn't run ruff format

For now I'm just using ruff check --fix ., which will fix the whitespace but not do other reformatting.

@alexrudd2 alexrudd2 mentioned this pull request Dec 1, 2024
@ssweber
Copy link
Author

ssweber commented Dec 1, 2024

@alexrudd2. I added in the tests to this branch and closed that PR. I was covering a coworkers job last week, but I should have time this week to run against a ClickPLC.

@ssweber
Copy link
Author

ssweber commented Dec 1, 2024

... getting my bearings on how to use pytest :)

Not sure how physical click would respond to setting all these as 1234. So revert.
@alexrudd2
Copy link
Owner

Nice catch on the SD ranges. (I cherry-picked it as a83d9b1).

I've invited you as collaborator so Github actions will run on this PR. Commit whatever you want in this branch, I'll likely squash-merge when we've got things fully tested.

@@ -145,6 +171,35 @@ async def test_dh_roundtrip(plc_driver):
await plc_driver.set('dh500', 500)
assert await plc_driver.get('dh500') == 500

@pytest.mark.asyncio(loop_scope='session')
async def test_sd_roundtrip(plc_driver):
"""Confirm writable SD ints are read back correctly after being set."""
Copy link
Owner

Choose a reason for hiding this comment

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

Let's split out the roundtrips for sd29 and sd35 into a separate test that fully sets the time, since that's what you actually care about.

At some point I expect there will be explicit functions for each of the 'special' SD registers.

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