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

def switch_control(): broken from merge with develop branch #22

Open
wasndas opened this issue Sep 14, 2021 · 4 comments
Open

def switch_control(): broken from merge with develop branch #22

wasndas opened this issue Sep 14, 2021 · 4 comments

Comments

@wasndas
Copy link
Contributor

wasndas commented Sep 14, 2021

It seems the concurrent control of multiple switches in def switch_control(): has been broken by a merge with the develop branch a while ago here

@changgonKim
Copy link
Collaborator

Hi Florian, I took a look at your comment, but it's hard to understand.
To make a asynchronous gathering and send the setState command concurrently, we used the asyncio.gather method.
Did you meant that this is wrong? Or do you mean that the setState method is colliding when we do that?

    try:
        tasks = []
        for switch in switches:
            tasks.append(asyncio.create_task(switch.setState(on, name, portnum)))

        await asyncio.gather(*tasks)

Thanks for your comments :)

@wasndas
Copy link
Contributor Author

wasndas commented Oct 12, 2021

Yes, right now its doing it sequentially.

for switch in switches:
        try:
            if command == "on" or command == "off":
                await switch.setState(on, name, portnum)

Since I also learned more about the asyncio stuff in the meantime, you can discard asyncio.create_task()

try:
        tasks = []
        for switch in switches:
            tasks.append(switch.setState(on, name, portnum))

        await asyncio.gather(*tasks)

Or if you do like comprehensions, you could do it in a one liner :)

await asyncio.gather(*[sw.setState(on, name, portnum) for sw in switches])

@changgonKim
Copy link
Collaborator

changgonKim commented Oct 22, 2021

Yes, but I remember that we had discussion about this. Turning on and off multiple switches at one time could be dangerous on the operation system, and also there should not be some situations that we need to turn on the multiple switches at the same time accurately. Therefore, I suggest we can just make the tasks on the upper level actors.

        tasks = []
        for i in range(7)
            tasks.append(command.send_message("lvmnps", f"on DLI-02 i"))

        await asyncio.gather(*tasks)

Should we have some missing points when we do this?

@wasndas
Copy link
Contributor Author

wasndas commented Oct 25, 2021

I think these are two different things, one is the actor capability of concurrently setting ports and usage - sure some hardware has to be switched on sequentially.
So you shouldnt push the concurrency to the upper level, doesnt make any sense.

Also I whould not use the port numbers, especially in the upper level software, on this level the used switch should even be irrelevant. Thats the reason why the name key from the config file should have a system wide unique name.

Another thing, the name of the port should onyl have [A-Za-z0-9_.-] and definitely no spaces !
eg:
name: "Hg-Ar Lamp"
more like:
name: "lvm.scp.lamp.hr-ag"
desc: "Hg-Ar Lamp of scp bla bla"

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

No branches or pull requests

2 participants