probonopd / WirelessPrinting

Print wirelessly from Cura, PrusaSlicer or Slic3r to your 3D printer connected to an ESP8266 or ESP32 module
352 stars 65 forks source link

Convert to a Library #115

Closed ayushsharma82 closed 5 years ago

ayushsharma82 commented 5 years ago

I think, It would be best to convert this into a Library with callback functions and variables accessed from C++ Class like Temperature, Status etc. It would provide a nice and easy interface for plugging in other libraries. More over, It keeps the clutter away from the user.

If we really want to expand this to common 3d printing users ( who have no experience in programming ), then it should not scare them at first.

Please let me know your views too.

probonopd commented 5 years ago

Wouldn't that make it much more complicated for us? I have never really written a library.

ayushsharma82 commented 5 years ago

Nope. It would be a lot organised. Just look at ESP-DASH on how to work with AsyncWebServer in a Class.

The goal of a library is to provide a simpler interface for users while not creating a roadblock in developing of their own code. Some users might want to add some functions of their own when using this project, it's really hard to do that in a cluttered environment where the project creator only knows 'what goes where'. A library just hides all that code for which a user doesn't need to take any tension.

probonopd commented 5 years ago

It's certainly worse once not even the creator knows anymore 'what goes where'. I am hesitant since I have only basic Arduino language skills (can write sketches and consume libraries), but I am not really a C++ expert.

ayushsharma82 commented 5 years ago

It's really not that hard.

class WirelessPrinting{
    public:

    private:
};

I will create a branch which is converted into a library. We will test this out incase it is better or not.

ayushsharma82 commented 5 years ago

So i have copy pasted the whole code into a Library ~ Class format. There are 4 ~ 5 errors on compilation, Any help would be appreciated.

https://github.com/ayushsharma82/WirelessPrinting/tree/library

GMagician commented 5 years ago

I suggest to create a singleton class, I think that no more than one instance will be ever used

ayushsharma82 commented 5 years ago

@GMagician I agree and i have done that by adopting Singleton Pattern

Used extern . https://github.com/ayushsharma82/WirelessPrinting/blob/library/src/WirelessPrinting.h#L131

Nope i was wrong, That's not Singleton Pattern :P , I will do changes according to this stackflow thread:

https://stackoverflow.com/questions/7923392/c-extern-class-declaration

GMagician commented 5 years ago

@ayushsharma82 you should declarte all methods and fields as static...

ayushsharma82 commented 5 years ago

If possible, Can anybody make me as a collaborator? I can make a branch on this repo and all collaborators can contribute in completing it. I am not familiar with Singleton classes, @GMagician contribution will be highly appreciated.

ayushsharma82 commented 5 years ago

Or @GMagician , You can clone my library branch in this repo.

GMagician commented 5 years ago

Can anybody make me as a collaborator?

I think is up to @probonopd add collaborators.

GMagician commented 5 years ago

@ayushsharma82 you may also add a pull request here, using your code, then we can work on it...

probonopd commented 5 years ago

I am not sure whether WirelessPrinting, being an application ("sketch") lends itself to being a library. A library is for use in other sketches. WirelessPrinting is not for use in other sketches, it is a sketch itself.

So before we make things more complicated (to the point that I don't understand it anymore - what is a singleton class?) let's really think about what the pros and cons are.

GMagician commented 5 years ago

Singleton class is a class with just one instance and usually without public constructor. Its instance is selfcreated by a method that return always the same instance. Don't know real pros/cons , I just gave my opinion about how to do it

ayushsharma82 commented 5 years ago

It will just help us in future to have a clean code base to work with. You have all functions in WirelessPrinting.h header file to get a quick overview, and WirelessPrinting.cpp with all of them declared. If we don't organize this into a library then, It will be just time consuming for future development where we will have to read the whole .ino file for 'what we have done so far'.

Another Advantage is Distribution, Once we convert this to a library, We can use Arduino Library Manager to distribute to 'masses' (people who don't understand github / programming ). Would be easier for them to keep the library updated too.

ayushsharma82 commented 5 years ago

@probonopd Can you create a library branch in this base repo? I will create a pull request to that 'library' branch. We can keep the branch open if it works otherwise we will just scrub the idea.

probonopd commented 5 years ago

Done: https://github.com/probonopd/WirelessPrinting/tree/library

ayushsharma82 commented 5 years ago

@probonopd Done

ayushsharma82 commented 5 years ago

@probonopd I think we should stick with ino file only. Even i don't have time nowdays to completely overwrite the whole thing.

Instead i will work on beautifying or sperating the current code like Marlin.

🙄

probonopd commented 5 years ago

@ayushsharma82 your help on beautifying the web interface would definitely be most highly appreciated :-) Let's make it a top example for ESP-DASH. :+1:

Please don't split things into too many files, personally I find it much harder then.

ayushsharma82 commented 5 years ago

Roger. I will only add esp-dash to it.

On Mon, 13 May 2019, 11:27 pm probonopd, notifications@github.com wrote:

@ayushsharma82 https://github.com/ayushsharma82 please don't split things into may files, personally I find it much harder then. But your help on beautifying the web interface would definitely be most highly appreciated :-) Let's make it a top example for ESP-DASH.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/probonopd/WirelessPrinting/issues/115?email_source=notifications&email_token=AGK4NSBT6LURFIWEF5YVV4TPVGTY5A5CNFSM4HIJX6Z2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVJCSHI#issuecomment-491923741, or mute the thread https://github.com/notifications/unsubscribe-auth/AGK4NSBXLWUFPTNQSOJELVTPVGTY5ANCNFSM4HIJX6ZQ .

TheAssassin commented 5 years ago

Might be useful to use the OctoPi protocol implementation on e.g., a Raspberry Pi Zero or cheap clones as a headless server with low overhead.