Update absolute position

[lromor]

closes #7 Implemented M114 command.

Now it should be possible to retrieve the precise actual position in realtime of the machine. pru_motion_queue should be able to callback in the free time motoroperations::status_listener so that for example if we are waiting for a slot to be freed, beagleg can still update the actual position and expose it in some other ways (tbd).

[bigguiness]

On Friday, May 27, 2016 4:52 PM leonardoromor wrote:

Implemented M114 command.

Now it should be possible to retrieve the precise actual position in realtime of the machine. pru_motion_queue should be able to callback in the free time motoroperations::status_listener so that for example if we are waiting for a slot to be freed, beagleg can still update the actual position and expose it in some other ways (tbd).

Hello,

Just my two cents... Henner may feel differently.

I think you should rebase this series a bit.

There are a number of commits that add files that are then removed later. This just clutters the git history and makes it harder to debug if something breaks later.

Each commit should either "fix" something or add a new feature. Having commits that just introduce your development steps make it harder to follow/review the series. The code should be able to compile and run properly after each commit. If it doesn't the commit needs to be rebased so that things are not "broken".

The "captians_diary.txt" shouldn't be included in the commits. Those are your development notes and have nothing to do with the repo code.

Don't change the BEAGLEG_HARDWARE_TARGET in the Makefiles. You can specify a different target in the make command line.

Don't commit build files to the repo (build/pip-delete-this-directory.txt).

I would like to review/test this properly but with all the clutter it's a bit hard to follow.

Over all I think the series is a good idea but currently it's a bit muddled. Like I said Henner may feel differently.

Regards, Hartley

[lromor]

@bigguiness Thank you a lot, sorry for the clumsy series, I will try to fix everything you said. I found out just now that usually people cleanup the git history before pull requesting, I feel very noob XD.

Regards, Leo

[hzeller]

Thanks for the cleanup. I agree with Hartley in all the points, and you seemed to have cleaned it up nicely (have not looked at the code yet). While working on stuff, there is a lot of work-in-progress, but having the final pull request one coherent submit with the minimal changes to make the particular feature work without changing unrelated things makes the review more smoohtly.

I'll do a first round of comments, and many will initially just be little style nits and later I'll go into the technical details

[hzeller]

Added a first round of comments; not yet looked too much into detail of the technical things.

There are some strategic points in which you should probably explain the feedback between PRU and host (the StatusRegister thing). And the assumptions you make (e.g. that a 32bit operation is atomic, hence the 24/8 bit split), so that it is easier for future readers to understand.

[hzeller]

(Ah, and don't worry, it looks like a lot, but I am also a pedantic code reviewer .. as you have seen in FlaschenTaschen :) ... most of the comments are minor)

[lromor]

There's also a new issue, if beagleg is run with dry run, the queue is updated but nothing is executed so at a certain point an assert complains because the PRU is not executing anything. Should I replace DummyMotionQueue with the MotionQueue used for the tests that handles a correct GetStatus() behavior?

(solved)

[lromor]

So, any other considerations left?

[mikikg]

Does this update also cover machine states like in GRBL, for example it would be very useful to have report on machine status, does machine is in RUN, IDLE or ERROR state? In many cases we will have request to wait until controller finish some movements so we can synchrony executes some other operation initiated from upfront application.

Also, it will be even more useful if controller can report state in even-driven fashion, to send automatically report when it finished some G command (if we requested somehow to do that) so we simply wait for event in upfront application to do other jobs, not to pull periodically that information about current controller state.

[hzeller]

@leonardoromor I am still on my way back from Toorcamp but will have stable internet again tonight. @mikikg One feature at a time :) But it does sounds like a brittle design of your application if you have to wait for gcdes to be finished to do something. I rather suggest to use the various commands that switch output pins when it is their turn; then you can use these to exactly switch whatever you have to do.

[mikikg]

@hzeller Tnx for reply. In my case I need to read some external Inputs and currently that is not supported by BeagleG. I need simple scenario likes, I move working peace "into machine" (hydraulic press) ant that can handle BeagleG but I need to wait in some point when press finish it's job (they will notify me) so I can "pull out" working peace. In that scenario I need synchronisation between motion controller and PLC operations, I can't rely on timing of movement, I need event (or to pull periodically) when motion controller finish it's job. That was the reason and request for such function in BegleG.

[hzeller]

After a couple of weeks away busy with other things, looked at the latest change. Looks much better. I have some superficial comments, but I need to take a more closer look tonight regarding the logic

[hzeller]

I think in general, it looks pretty cool. I have a number of comments mostly around:

• Documentation. You need to express your intend when adding types and members. While it might be clear while writing, you need to be able to pass on the idea to someone reading the code (and yourself after not looking at it for a while)
• Also Documentation and possibly simplification: it is not entirely clear who is calling what up and downstream. Maybe the MotionQueue could even keep track of the counts whenever asked, so that it has not to call up ?
• Complexity: You go out of your way to use the RingDeque to keep track of the history, but you're breaking abstraction of the RingDeque in the course of that. Also, it is very hard to understand what is going on because of that. Rather, try to just keep track of the history in a separate queue, similarly handled like the PRU motion queue (i.e. 'manual' RingDeque) and keep everything fixed to that. Either by having Enqueu() return an index to where things are and keep a 'shadow' array with the history information (and also, with that index the StatusRegister makes sense, because the index refers to somnething real.). Or even have PRUMotionQueue::Enqueue() allow to store an arbitrary pointer object alongside and make that accessible via the index. Your call :)

[hzeller]

Are there updates here ?

[lromor]

Not yet, I hope to have some time this weekend to work on it

[hzeller]

Pulling out the PRU code looks very good, pretty much exactly how I'd done it (of course, I still have some comments :) ). Can you make this a separate pull-request, so that we can integrate that already ?