Port of new comms protocol from Makera Controller 0.9.12 by SergeBakharev · Pull Request #341 · Carvera-Community/Carvera_Controller

SergeBakharev · 2025-08-21T01:43:29Z

No description provided.

SergeBakharev · 2025-08-21T02:33:49Z

+            if not app.playing and (not self.echosended) and (not self.echosending):
+                try:
+                    if self.controller.stream:
+                        self.echosending = True
+                        self.controller.stream.send(b'echo echo\n')
+                        echo = self.controller.stream.getc(10)
+                        if echo == b'echo: echo':
+                            self.message_popup.lb_content.text = tr._('Firmware version mismatch! \nPlease use a Controller with version V0.9.11 or earlier \nto upgrade the firmware to V1.0.4.')
+                            self.message_popup.btn_ok.disabled = False
+                            self.message_popup.open(self)
+                            return
+                        self.echosended = True
+                except:
+                    print(sys.exc_info()[1])


We have better ways of checking the firmware version since we directly have app.is_community_firmware and app.fw_version_digitized

Please replace for something easier to read that uses those two app attributes.

SergeBakharev · 2025-08-21T02:36:01Z

+                curr_dir = os.path.join(sandbox_documents_path, 'gcodes')
+                local_path = os.path.join(os.path.dirname(curr_dir), remote_post_path)
+            else:  # inserted
+                local_path = os.path.join(os.path.dirname(os.path.realpath(__file__)), remote_post_path)


Unlike Makera our controller actually uses temp directories:
Please continue to use local_path = os.path.join(self.temp_dir, remote_post_path)

SergeBakharev · 2025-08-21T02:38:04Z

            #md5 = Utils.md5(self.uploading_file)
            md5 = Utils.md5(displayname)
            self.controller.uploadCommand(os.path.normpath(remotename))
+            time.sleep(0.2)


This is arbitrary. Some time needs to be given for the machine decompress the file but it shouldn't be a fixed 0.2s.

SergeBakharev · 2025-08-21T02:41:09Z

                md5 = Utils.md5(tmp_filename)
            self.controller.downloadCommand(self.downloading_file)
-            self.controller.pauseStream(0.2)
+            self.controller.pauseStream(0.0)


What does this do

SergeBakharev · 2025-08-21T02:43:07Z

+            if kivy_platform == 'ios':
+                import tempfile
+                tmp_file = tempfile.gettempdir()
+                config_path = os.path.join(tmp_file, 'config.txt')
+            else:  # inserted
+                config_path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'config.txt')


Unlike makera we use temp directories, so this conditional business is not required. Please revert to config_path = os.path.join(self.temp_dir, 'config.txt')

SergeBakharev · 2025-08-21T02:43:33Z

+        if kivy_platform == 'ios':
+            import tempfile
+            tmp_file = tempfile.gettempdir()
+            app.selected_local_filename = os.path.join(tmp_file, 'config.txt')
+        else:  # inserted
+            app.selected_local_filename = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'config.txt')


Same as cda5887#r2289723789

SergeBakharev · 2025-08-21T02:44:12Z

+            local_path = join(expanduser('~'), 'Documents')
+            local_path = join(local_path, remote_post_path)
+        else:  # inserted
+            local_path = os.path.join(os.path.dirname(os.path.realpath(__file__)), remote_post_path)


Should use our version local_path = os.path.join(self.temp_dir, remote_post_path)

SergeBakharev · 2025-08-21T02:45:30Z

+    echosended = False
+    echosending = False


This is used for some reason by makera as a way of version checking. Per cda5887#r2289714852 We have a better way. Please remove it.

SergeBakharev · 2025-08-21T02:48:08Z

                size = None
-
-            print(('putc(', repr(data), repr(size), ')'))
+            print(("putc(", repr(data), repr(size), ")"))


No print statements for debugging please. Use logger.debug()

SergeBakharev · 2025-08-21T02:48:22Z

                data = None
-
-            print(('getc(', repr(data), ')'))
+            print(("getc(", repr(data), ")"))


cda5887#r2289728438

SergeBakharev · 2025-08-21T02:50:59Z

-__author__ = 'Wijnand Modderman <maze@pyth0n.org>'
-__copyright__ = ['Copyright (c) 2010 Wijnand Modderman',
-                 'Copyright (c) 1981 Chuck Forsberg']
-__license__ = 'MIT'
-__version__ = '0.4.5'


Let's not remove the acknowledgement to the original author

SergeBakharev · 2025-08-21T02:51:23Z

-
-    '''
-
-    # crctab calculated by Mark G. Mendel, Network Systems Corporation


Let's not remove the acknowledgement to the original author

SergeBakharev · 2025-08-21T02:53:40Z

    def download(self, filename, local_md5, callback):
        stream = open(filename, 'wb')
-        result = self.modem.recv(stream, md5 = local_md5, retry = 10, callback = callback)
+        result = self.modem.recv(stream, md5=local_md5, retry=50, callback=callback)


... 50 retries seems excessive. Users will think the app is broken if it spends this much time retrying. Let's continue using the original value of 10, or maybe even reduce it to 3.

SergeBakharev · 2025-08-21T02:53:48Z

        # do upload
        stream = open(filename, 'rb')
-        result = self.modem.send(stream, md5 = local_md5, retry = 10, callback = callback)
+        result = self.modem.send(stream, md5=local_md5, retry=50, callback=callback)


cda5887#r2289733565

SergeBakharev · 2025-08-21T02:55:58Z

        self.t = None
        self.tr = None
-
+        return


init methods does not explicitly return a value. Its purpose is to initialize the attributes of a newly created object, not to produce a return value for assignment.

SergeBakharev · 2025-08-21T02:57:05Z

 UDP_PORT = 3333
 BUFFER_SIZE = 1024
-SOCKET_TIMEOUT = 0.3  # s
+SOCKET_TIMEOUT = 5


The comment that this is in seconds is useful. Often high speed actions like this are measured in milliseconds

SergeBakharev · 2025-08-21T02:58:54Z

+        try:
+            self.socket = socket.socket(family=socket.AF_INET, type=socket.SOCK_STREAM)
+            ip_port = address.split(':')
+            self.socket.settimeout(3)


Eh? Whats the point of defining SOCKET_TIMEOUT as 5 if it's not even used.

SergeBakharev · 2025-08-21T02:59:17Z

+            ip_port = address.split(':')
+            self.socket.settimeout(3)
+            self.socket.connect((address.split(':')[0], int(address.split(':')[1]) if len(ip_port) > 1 else TCP_PORT))
+            self.socket.settimeout(SOCKET_TIMEOUT)


I'm confused. Why set a socket timeout after the connection?

SergeBakharev · 2025-08-21T03:07:47Z

It's a bit weird that one of the changes here is to convert values that were previously in hex to base-10. The convention for most low level protocol work is in hex. It does not harm, just makes the file changes look more than it really is.

SergeBakharev

There is a lot of iOS related changes here, that I'm not comfortable commenting on. I don't know if they conflict with the existing stuff written by @zittix or not. I don't like the idea of taking in code changes from Makera without verifying them, so I would prefer to drop all the iOS changes.

I see some re-occuring issues:

Makera is using some inane way of testing the firmware version for some reason with controller.echosending We shouldn't propagate that when we already do better with our own MakeraApp.is_community_firmware and MakeraApp.fw_version_digitized
We use a temp_directory for temporary files but Makera is saving the files directly next to the controller executable. We should continue to use MakerApp.temp_dir

A point of discussion: This change is breaking, are we okay with that? Previous versions of firmware will not be supported. Which is not ideal but I'm okay with. We should however make it clear in our messaging the order that the updates should be applied: First new firmware uploaded with previous controller version before using the new controller.

Sorry I wrote my previous comments while looking at a specific commit so I wasn't able to directly suggest code changes.

SergeBakharev · 2025-09-18T01:45:14Z

We should put this PR on ice. Makera have pulled 0.9.12 because it's got some critical bugs. I worry that those bugs are protocol design related.

zittix · 2025-09-18T10:58:06Z

IMHO we should keep backward compatibility at least during one controller version to allow people to upgrade/downgrade to avoid things like Makera is doing "please first use that version to upgrade then upgrade to that one".

WARIO2412 added 2 commits August 21, 2025 01:46

fix LEAD mode for the pendant

4ed6cdc

Update to new protocol

cda5887

SergeBakharev commented Aug 21, 2025

View reviewed changes

SergeBakharev marked this pull request as ready for review August 21, 2025 03:20

SergeBakharev marked this pull request as draft August 26, 2025 07:55

benjgray mentioned this pull request Aug 27, 2025

iOS - build failing - 'libffi' patch fails with "hunks failed" #345

Closed


		'''

		# crctab calculated by Mark G. Mendel, Network Systems Corporation

Conversation

SergeBakharev commented Aug 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeBakharev left a comment

Choose a reason for hiding this comment

Uh oh!

SergeBakharev commented Sep 18, 2025

Uh oh!

zittix commented Sep 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants