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

serialplotter filter #4058

Closed
wants to merge 8 commits into from
Closed

serialplotter filter #4058

wants to merge 8 commits into from

Conversation

yhur
Copy link

@yhur yhur commented Sep 20, 2021

I'm publishing a package called serialPlotter which the serial data just like the Arduino IDE's serial plotter.

And this pull request is an addition of new pio device monitor filter that invokes the serialPlotter so that the PIO developers can have the Arduino IDE's serial plotter equivalent.

Regards,

@CLAassistant
Copy link

CLAassistant commented Sep 20, 2021

CLA assistant check
All committers have signed the CLA.

@yhur
Copy link
Author

yhur commented Sep 20, 2021

Needs some fix, so I'm closing the PR.

@yhur yhur closed this Sep 20, 2021
@yhur
Copy link
Author

yhur commented Sep 20, 2021

I'm publishing a package called serialPlotter which the serial data just like the Arduino IDE's serial plotter.

And this pull request is an addition of new pio device monitor filter that invokes the serialPlotter so that the PIO developers can have the Arduino IDE's serial plotter equivalent.

Regards,

I'm reopening the PR after fixing the cleanup procedure.

The usage of this function is as follows.

pio device monitor -f serial_plotter 

And I would like to get some advice on how to get the prereq(https://github.com/yhur/PIOSerialPlotter.git) installed automatically when the user invokes above command for the first time if possible.

Thanks.

@yhur yhur reopened this Sep 20, 2021
@yhur
Copy link
Author

yhur commented Sep 22, 2021

And I would like to get some advice on how to get the prereq(https://github.com/yhur/PIOSerialPlotter.git) installed
automatically when the user invokes above command for the first time if possible.

For now, I put the preventive code to handle the case the serialPotter is not installed yet by the commit 'handling for the case no serialPlotter installed.

def __init__(self, *args, **kwargs):
super(SerialPlotter, self).__init__(*args, **kwargs)
self.buffer = ''
self.plotter_py = os.path.expanduser('~') + '/.platformio/packages/tool-serialplotter/serialPlotter.py'
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work for Windows users? Better not use os.path.join() to concatenate paths?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @maxgerhardt ,
Thank you for the suggestion. I'll modify the code on my branch for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, in fact it can't be assumed that PlatformIO is even installed in that path since it can be arbitrary. You should use get_project_core_dir() per

"PLATFORMIO_CORE_DIR": get_project_core_dir(),

(from platformio.project.helpers import get_project_core_dir)

Copy link
Author

Choose a reason for hiding this comment

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

Great! Actually that's what I really needed. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

By the way, since I started changing the serialPlotter tool itself after getting @ivankravets's advice, I may change some part of this filter to take the tools modification. And I may work on it this Weekend. So let's continue this thread early next week. Thanks.

Copy link
Author

@yhur yhur Oct 2, 2021

Choose a reason for hiding this comment

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

Hi @ivankravets and @maxgerhardt ,

I reworked the serialplotter python command and it’s renamed to arduplot taking @ivankravets idea of naming.
Github repo → https://github.com/yhur/arduplot

  1. And I pushed it to pypi.org, so it can be installed with pip.
  2. this serial_plotter filter is also updated.

Would you take a look and give me your feedback for the merge?

@maxgerhardt
Copy link
Contributor

This needs the help of @ivankravets to integrate and auto-install your suggested tool-serialplotter package by default in PlatformIO.

@yhur
Copy link
Author

yhur commented Sep 29, 2021

This needs the help of @ivankravets to integrate and auto-install your suggested tool-serialplotter package by default in PlatformIO.

yes @maxgerhardt , I had a talk with @ivankravets already, and am working on my tool-serialplotter as a pip-installable. I will come back to you and him after working on that too. Thanks.

@yhur
Copy link
Author

yhur commented Oct 2, 2021

Hi @ivankravets and @maxgerhardt,
I realized I overlooked the difference of the loaders of Windows and other Unix like OSes which uses the first couple of bytes of the files to determine the loading and invoking of the program while Windows depends on the extension.

When researched other packages like ‘twine’, it installs twine.exe for Windows while twine python script for MacOS/Linux. So I think I need to make ‘arduplot’ as ‘arduplot.exe’ for Windows, but don’t know how to do it.

Could you help on this or give me some pointers to refer and to learn? Thanks.

@yhur
Copy link
Author

yhur commented Oct 3, 2021

Hi @ivankravets and @maxgerhardt,

As an alternative resolution to the 'arduplot' command for Windows, I wrapped it with 'arduplot.cmd' batch script.
And since the terminating of the monitor and/or plot window is different between Windows and MacOS/Linux, some change was also added. I'm going to pause here and wait for your feedback. Thanks.

Comment on lines +41 to +46
if sys.platform == 'win32':
self.arduplot = os.path.join(pio_root, 'penv', 'Scripts' , self.arduplot + '.cmd')
else:
self.arduplot = os.path.join(pio_root, 'penv', 'bin' , self.arduplot)

if not os.path.isfile(self.arduplot):
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to call arduplot via subprocess and depend on the physical files. Please import arduplot directly as a module

import subprocess 
from platformio import proc

try:
	from arduplot import main as arduplot_main
except ImportError:
    args = [proc.get_pythonexe_path(), "-m", "pip", "install", "--upgrade", "arduplot"]
    subprocess.call(args)
    from arduplot import main as arduplot_main


arduplot_main(**kwargs)

Please note that you will need to split code in https://github.com/yhur/arduplot/blob/main/src/arduplot/plotserialdata.py#L57

For example, keep function main without any changes and create a new one:

@click...
def cli(**kwargs):
   arduplot_main(**kwargs)

P.S: I didn't test this code. Just gave you a few ideas where to move further. Thanks!

See

Copy link
Author

@yhur yhur Oct 15, 2021

Choose a reason for hiding this comment

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

Thank you for your review.
The reason I call arduplot via subprocess is to use the monitor feature's serial printing as well while plotting.
Please take a look at the video.

And regarding creating an additional function, could you please tell me what that means or any link that explains the 'cli' function? Like what it does in addition to the main and so on. Sorry I'm new to click.

My.Movie.1.mp4

@ivankravets
Copy link
Member

We finally implemented custom device monitor filters. See docs https://docs.platformio.org/en/latest/core/userguide/device/cmd_monitor.html#custom-filters

So, let's move this discussion to yhur/arduplot#2

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.

None yet

4 participants