intelligent-agent / redeem

Firmware for Replicape
http://wiki.thing-printer.com/index.php?title=Redeem
GNU General Public License v3.0
36 stars 44 forks source link

Rebrand pipe end into /dev/redeem/ #173

Closed Wackerbarth closed 5 years ago

Wackerbarth commented 5 years ago

Placing all of the pipe ends in a common directory within /dev provides a more descriptive namespace entry.

It will help simplify future maintenance of the connections.

Note that this change MUST be coordinated with corresponding changes to the configuration files for OctoPrint and Toggle

ThatWileyGuy commented 5 years ago

... but why?

The current location hasn't ever caused us any issues and there won't be any future maintenance anyway because we'll almost certainly be building socket support for OctoPrint 1.4.

This seems to introduce a breaking change across multiple components that doesn't change anything user-facing and has no long term benefit?

Wackerbarth commented 5 years ago

Why? -- Because it is more maintainable to have all of the endpoints grouped together. And the renaming also creates an easier-to-identify purpose in the /dev/ space. Although we are likely going to add alternative socket access points for use with OP, I'm not at all convinced that we will be eliminating all of the pipe endpoints. Organization of the namespace is a long term benefit.

No, the benefit may not be obvious or immediate because we have adapted to a particular situation. But, things are not totally static and adopting a better convention now will simplify things later.

The proper way to implement this change would first evaluate the technical correctness when the associated changes are taken together. The actual deployment would then be associated with the building of a new image and all subsequent ones.

goeland86 commented 5 years ago

I agree with @ThatWileyGuy that the change doesn't seem warranted. Once we have socket support, there will no longer be a need for any serial connection at all. The entire terminal input/output exchange will happen via a socket interface.

Renaming now brings nothing advantageous besides a further change to users' existing setups where they'll need to edit their octoprint configs to change the additional serial port listing.

If we do this at all, we do it after the 2.2.x release is out the door, to give us time to: a) properly sync all affected modules b) build an update script to modify existing configs to fit the new software schema

Wackerbarth commented 5 years ago

I don't think that adding, and utilizing Unix sockets for communication eliminates the need to preserve the capability of using pipes for communication. Particularly because of the restart overhead involved, I may want to avoid having OctoPrint involved in my local printer and relegate it to another device that "monitors" more than "controls" most operation. The exact nature and protocol of the inter-module communication should be modular and support either kind of channel.

Wackerbarth commented 5 years ago

As to "when" a change such as this should be made, it depends on just how much backward compatibility is useful. I can argue that if the user will need to reflash anyway, just BEFORE that release is a better time to introduce the change. Since the configuration change is associated with files handled by Umikaze, it will affect only users who are already overriding the image-provided installation files. I seriously doubt that there are many who touch these files anyway.

ThatWileyGuy commented 5 years ago

Not all users reflash, and without this change, there's currently nothing in the pipe that requires a reflash. I'm running the latest Redeem on an old 16.04 image on one of my printers because it's a pain to get to the MicroSD slot.

What "maintenance cost" do you believe the current path has? I can't say I've ever spent any time at all that moving the device path would alleviate.

Wackerbarth commented 5 years ago

I envision that in some future revision, at least some of the pipes, etc. will be generated on the basis of a request in the configuration. Having them clustered in the /dev tree makes it easier to identify and otherwise work with them in a "wizard" program.

As to the issue of re-flashing, at some point we will need to assure that the user has an "approved" kernel, and all of the virtual environment infrastructure in place. Given the history of the past year, and the multiple times that you have made us change kernels, I would think that you would want to require everyone to upgrade. This is a new major release. In general, users will expect significant changes. Letting it "slide" will just make it politically more difficult to introduce a class of changes in the future.

ThatWileyGuy commented 5 years ago

I envision that in some future revision, at least some of the pipes, etc. will be generated on the basis of a request in the configuration. Having them clustered in the /dev tree makes it easier to identify and otherwise work with them in a "wizard" program.

We have exactly one Redeem pipe that is actively used and I have seen no demands for more. That pipe is about to be removed in favor of a socket for OctoPrint 1.4 because that's easier for us to reason about. I believe this will ultimately end with us deprecating pipe support completely.

As to the issue of re-flashing, at some point we will need to assure that the user has an "approved" kernel, and all of the virtual environment infrastructure in place. Given the history of the past year, and the multiple times that you have made us change kernels, I would think that you would want to require everyone to upgrade. This is a new major release. In general, users will expect significant changes. Letting it "slide" will just make it politically more difficult to introduce a class of changes in the future.

In general, no, there is no hard requirement that users upgrade their kernels. I have only "made" us switch kernels in an effort to keep up with what's currently shipping and supported (or even available). Doing so lets us quietly take advantage of the bugfixes and features that thousands of developers contribute to the Linux kernel. If nothing else, USB on the BBB has become immensely more stable in recent versions.

Wackerbarth commented 5 years ago

"We have exactly one Redeem pipe that is actively used" -- That is not true. Perhaps YOU use only one. However, Toggle also uses a pipe and, since I find OP as much a hinderance as helpful, I routinely feed instructions directly. This gets back to my comment that this is a toolbox rather than a printer. What I am trying to say is that different users utilize the components in different ways. We have different kinematic configurations, different sensors, etc. If you restrict your thinking to only one configuration, you make the resulting "product" significantly less useful.

As far as supporting multiple kernel series, etc. I find that argument far less compelling. As you note, the stability of the kernel is continually evolving. However, the functionality that the kernel provides to a controller is essentially constant. As a result, I see little reason to attempt to maintain backward compatibility across multiple kernel families. Periodically dropping legacy support is a reasonable policy.

goeland86 commented 5 years ago

@Wackerbarth Toggle does not, in fact, use the pipe directly to redeem. It never has. It was an integration that was envisioned, but really, it's just something "we may want one day" that we never used. It's been there for years. Prune the dead code.

That leaves us with exactly one pipe - which is used by OctoPrint.

The fact that you use a secondary pipe to send Gcode to redeem separately is frankly just a shortcut to typing it through the OctoPrint terminal. I'm not saying it's wrong, but the functionality would be maintained by using the OctoPrint UI. Or, connect to the socket yourself and interact at that level. I see no reason to put in any effort in refactoring what's essentially going to be a dead end as soon as OctoPrint 1.4.0 drops.

Wackerbarth commented 5 years ago

I don't see it as a dead end. In fact, maintaining the existence of a pipe, but actually generating it only optionally when specified in the configuration file, makes even more sense. As we discussed previously, there is a reasonable use case for alternate front ends. I think that it would be a big mistake to limit the options. After all, sockets, pipes, etc. are just particular implementations of something that, generically, might be called a "control channel".

goeland86 commented 5 years ago

While I agreed there may be a use-case for a different front-end on Toggle, please don't misconstrue what I said. We bundle OctoPrint because it's a very complete control interface to a printer, that's customizable etc. If you want to start offering bundles for different systems, fine, but do it in a fork different from the main repository.

There is, at present, no viable OSS solution to replace OctoPrint with. Further, you keep insisting on making things more generic, when, in fact, I think we should focus on providing an almost exclusively integrated solution. The code is open. If you want to have a fork that makes things generic, go ahead. @ThatWileyGuy, @eliasbakken and I will continue focusing on the stack as it's been defined for the project: AM335x + Linux + Redeem + OctoPrint + Toggle. That is what we are trying to offer for the users to work with.

I'm starting to get really frustrated by your insistence on making things generic when, in fact, there's been no demand by anyone other than you for that. It's fine for you to make personal forks to make things generic and usable with other components - but we will not integrate that in the intelligent-agent repository without being extremely picky.

ThatWileyGuy commented 5 years ago

Let me put it this way - if we move to sockets and deprecate pipes, you'll have to switch from opening a pipe to opening a telnet console. Would that be a major reduction in functionality, and if so, why?

eliasbakken commented 5 years ago

I concur with Wiley and Jon on his, no need to make things more generic in order to support possible future changes. It's better to cut down on the scope to keep it lean to the platform we have. If there is a need to expand into other platforms, I say we cross that bridge when we get there.

As for the pipes, they can be removed if there is finally a socket solution from OctoPrint. It was always a crutch to circumvent the fact that OctoPrint needed a tty to talk to. Toggle did initially use a pipe directly to talk to redeem, but it was removed in favor of the REST and websocket interfaces on OctoPrint.

Great idea with the telnet alternative. It's been a while since I had to pipe G-codes from the command line, but it would be good to have an alternative still!

Kind regards, -Elias

Den lør. 9. mar. 2019 kl. 17:21 skrev Andrew Wiley <notifications@github.com

:

Let me put it this way - if we move to sockets and deprecate pipes, you'll have to switch from opening a pipe to opening a telnet console. Would that be a major reduction in functionality, and if so, why?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/intelligent-agent/redeem/pull/173#issuecomment-471197203, or mute the thread https://github.com/notifications/unsubscribe-auth/ADlQ2S-NyqsmrW8Oy05IjSoKFe6qjx7aks5vU9-GgaJpZM4aCIJ6 .

Wackerbarth commented 5 years ago

"Let me put it this way - if we move to sockets and deprecate pipes, you'll have to switch from opening a pipe to opening a telnet console. Would that be a major reduction in functionality, and if so, why?"

It is a reduction in functionality because the "telnet" protocol generally, is not, the protocol that is used. Requiring it would mean that the program on the other end of the pipe would have to be changed. Look at it this way. -- Assume that, rather than using OP, we were using another program "PrinterHost" that uses some connection other than a pipe. Therefore, you COULD remove this code that supports pipes. Then, if you had done so, how would you respond to the request to support OP 1.3.10?

Wackerbarth commented 5 years ago

@ThatWileyGuy -- As to your comment about re-flashing, I will remind you that you indicated that our typical response when someone reports a problem is that we ask them to upgrade to the "current version" of everything. And the easiest way to do that is to re-flash. Therefore, introducing a "breaking change" actually helps us assure that the users are more likely to be running a supported combination and not one where only some of the system has been upgraded. As we know, there are problems mixing the wrong kernel or certain libraries with our "up-to-date" code. This change would not hurt anyone upgrading appropriately and would discourage having users who only partly upgrade.

ThatWileyGuy commented 5 years ago

I believe we reached a consensus that this is a purely aesthetic improvement that will cause problems and solve nothing.