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 serial connection #34

Merged
merged 1 commit into from
May 17, 2024
Merged

Add serial connection #34

merged 1 commit into from
May 17, 2024

Conversation

marcelldls
Copy link
Contributor

I tested this against both a virtual serial device (based on https://stackoverflow.com/questions/52187/virtual-serial-port-for-linux) as well as some hardware (Thorlabs MFF usb serial device). To test virtually:

Create virtual serial port in terminal 1

socat -d -d pty,raw,echo=0 pty,raw,echo=0

Returns:

2024/05/13 09:06:37 socat[14186] N PTY is /dev/pts/9
2024/05/13 09:06:37 socat[14186] N PTY is /dev/pts/10
2024/05/13 09:06:37 socat[14186] N starting data transfer loop with FDs [5,5] and [7,7]

Connect controller in terminal 2

test-demo # port="/dev/pts/10" - DiamondLightSource/demo-fast-cs#11

Send serial device response in terminal 3

Test the position AttrR (can inspect via Phoebus screen)
echo -e -n "\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01" > /dev/pts/9
or echo -e -n "\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02" > /dev/pts/9

Monitor port in terminal 4 (optional)

cat -v /dev/pts/9

Can see what the controller sends

@marcelldls marcelldls requested a review from GDYendell May 13, 2024 15:27
Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 43.91%. Comparing base (2c46785) to head (7eb9da1).
Report is 2 commits behind head on main.

Files Patch % Lines
src/fastcs/connections/serial_connection.py 50.00% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #34      +/-   ##
==========================================
+ Coverage   43.47%   43.91%   +0.44%     
==========================================
  Files          18       20       +2     
  Lines         651      690      +39     
==========================================
+ Hits          283      303      +20     
- Misses        368      387      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@GDYendell GDYendell left a comment

Choose a reason for hiding this comment

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

I think this looks good. Just a couple of minor suggestions.

src/fastcs/connections/serial_connection.py Show resolved Hide resolved
src/fastcs/connections/serial_connection.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@marcelldls marcelldls marked this pull request as ready for review May 16, 2024 12:44
@GDYendell
Copy link
Contributor

This is good now, but could you squash everything into one commit? Changing the dependency list and then changing it back again is still losing the history. I think the easiest way to do this (besides lazygit) is

git reset --soft ed95aec
git commit --amend --no-edit

@marcelldls
Copy link
Contributor Author

This is good now, but could you squash everything into one commit? Changing the dependency list and then changing it back again is still losing the history. I think the easiest way to do this (besides lazygit) is

git reset --soft ed95aec
git commit --amend --no-edit

Im happy to do this - Although I would suggest the easiest way is "Squash and merge"

@GDYendell
Copy link
Contributor

GDYendell commented May 17, 2024

With squash and merge you don't get a merge commit. Or rather it turns your commit into a merge commit and puts it on top, so there is not visible merge in the graph.

Expand dependancy list

rename aioserial_instance to stream
@marcelldls
Copy link
Contributor Author

As you wish. BTW the easy way IMO is git rebase main -i

@GDYendell GDYendell merged commit fb929a1 into main May 17, 2024
17 checks passed
@GDYendell GDYendell deleted the add-serial-connection branch May 17, 2024 09:03
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