Skip to content

added custom model and board - added reverse logic endstop#50

Open
MicioMax wants to merge 3 commits intoOpenScan-org:mainfrom
MicioMax:miciomax
Open

added custom model and board - added reverse logic endstop#50
MicioMax wants to merge 3 commits intoOpenScan-org:mainfrom
MicioMax:miciomax

Conversation

@MicioMax
Copy link

As from Discord post, I added the ability to set a "custom" device and shield, in order to allow custom models.
I also added an "active_high" endstop parameter to use endstops with reverse logic (high when pressed instead of low).

@esto-openscan
Copy link
Contributor

Thanks for the PR!

Custom builds (CUSTOM enums + MicioMax config)
The CUSTOM enum values look like a nice addition for supporting custom builds. Regarding the MicioMax device config, I’m not sure we should ship it as a default config in the repo, otherwise every custom PCB might need its own file. Would you be okay adding it as example_custom.json instead? I think this would be a great starting point for anyone with custom hardware.

Endstop / active_high 
You mentioned the switch is "high when pressed". With pull_up=True, the callback is still wired to when_released, so the trigger timing might not match your intent. Could you quickly double-check whether the motor stops when the endstop is hit, not when it is released? If it stops on release, we should switch between when_pressed and when_released based on active_high.

move_to_endstop()
This duplicates parts of move_degrees() but misses some safety guards. The main concern is the missing _stop_requested = False reset. If a previous stop() was called (e.g. by an endstop trigger), the motor would silently skip the entire movement. The is_busy() guard is less critical since the router already checks it, but good to have for when move_to_endstop() gets called from elsewhere.

Side note: the homing flow (initial angle override, hardcoded degrees, sleep(3)) and even _execute_movement() could use cleanup. I can follow up separately so we keep this PR focused.

@MicioMax
Copy link
Author

Hi, some notes on yours and more enhancements:

Custom builds (CUSTOM enums + MicioMax config)
It's ok to put in examples, I didn't know about them and I let inside default configs to show usage of active_high

Endstop / active_high
My switch is a "normally closed" one, so it opens when pressed. When wired as yours (pull up resistor and connection to ground) brings high (1) level when pressed and low (0) level when released, so you need the active_high. I tested it and it works correctly. I could also wire the endstop differently, so with a pull-down resistor and wired to positive rail instead of negative, so the "active_high" flag would not be needed; but then you should wire differently the endstops basing on their kind.
Resuming : if you take my code, with NO "active_high" flag it behaves the old way; with "active_high" enabled it stops the motor on 1 level, so when switch opens if wired like usual.

move_to_endstop()
the safety guard should be the endstop itself, maybe we can limit the number of steps to do a full 360° turn or, at least, 180°. IIRC you have 140° maximum, which if started when the scanner is pointing to 0° is not enough.
IMHO the "home" command should deserve an API endpoint and its behaviour should be set by some flags in the device settings, for exampe "home_mode=normal | calibrate_at_startup_only | calibrate_when_motors_idle" | calibrate_always" so it could cover all usages. To be on the safest side we'd need TWO endstops.... my PCB has connectors for them and doing so we could even measure the max and min device positions. A bit overkilling, maybe.

timeouts
simple motor timeout could be "timeout=0" for no timeout and "timeout=nnn" for nnn seconds motor timeout.
If we want to go deeper, we could put a "motor_timeout" and a "device_timeout" which puts the whole device, including the raspberry, in sleep mode. A bit overkilling, but it could spare some CO2 if the device is left alone for hours ;-)

device discovery in network
I was playing yesterday with device discovery on my K1Max, I got to appear it as a printer device which, when double clicked on it, opens the browser to the main page. On K1Max I had to use a python script because the provided wsdd2 service is lacking, but on raspberry we could have both the network shares as "computer" and the web page as "scanner device" with some settings tweaking, which would be nice... it's simpler to open the "network share" page and double click on the scanner device.

@esto-openscan
Copy link
Contributor

Awesome and thanks for pushing this forward!

Also, the additional feature ideas here and the process you shared on Discord sound really cool.

How would you like to proceed with this and the open PR?

I think we could merge this fairly quickly with a few small changes (e.g. config naming and resetting _stop_requested). If you're okay with it, I'm happy to suggest the changes directly on your PR (or push a small follow-up commit), so we can keep the momentum and get it merged.

For the newer feature ideas: do you prefer opening separate PRs for those, or should we turn the current PR into a draft and merge it later as a bigger package?

@MicioMax
Copy link
Author

Hi,
I think is better to wait and merge a bit later, as I've ready (just testing a bit more) the sleep mode stuff and I plan to add the PWM control (probably an endpoitn /light/light name/dim with a value from 0 to 100% and a dedicated HOME endpoint (so we can let the backend choose which kind of home depending on config values).
Later I could also implement the LCD display with the ability to do "offline scans".
It's ok for you ?

@esto-openscan
Copy link
Contributor

That sounds great, and I really like the direction.

I think it makes sense to wait a little and merge after the motor sleep mode and home endpoint are included. Those two features are closely related, and merging them should still be manageable.

For the other ideas (PWM light control, LCD + offline scans), I’d prefer separate PRs afterwards to keep the review smaller and avoid too much divergence and merge conflicts with the develop branch.

So let’s proceed like this: we keep this PR open (or convert it to draft while you finish testing), add sleep mode + home endpoint, then we merge this PR and continue with PWM / LCD in follow-up PRs.

If you need any help while testing or wiring this up, feel free to ping me.

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