Jiri Baum wrote:

> Hello Mario,
>
> this discussion probably belongs on the list; I've no objection if you
> reply to this on-list.
>
> > > Glancing through the TODO:
>
> > > 8: I'm not sure what you did here - as far as I know, while and for
> > > loops are identical in all respects (except syntax).
>
> > Well, I changed the while no error is found into a for every value in the
> > linuxplc.conf file, and skip/ignore errors. Its my very own
> > 'shorthand'... Sorry for using it on a public document.
>
> OK, no worries. Though I'd have thought an error in the config would
> warrant an error message rather than just a warning (commented out)...

I had not yet written the log file library when I did this change. Never got around to uncommenting the warning log...

> (Hmm, that probably points to a deeper problem: is there policy for error
> and warning messages?)

Yes. When I finished the logging library I mailed the list suggesting a discussion should be made to come up with a policy of when and how to log messages. It never started. Maybe we can start it now?

> > > 16: probably not. Same with PLClogfile and PLCdebug (though those
> > > perhaps have more justification).
>
> > I'm having a change of heart. Leave them as possible overides of what is
> > defined in the conf file. What do you think? Maybe just let them be until
> > somebody complains...
>
> I'm not sure. In some ways having overrides for some values but not for
> others is confusing (because inconsistent). If we want overrides, we should
> probably have a general syntax, --section:variable=value or something, but
> I can't see how that'd be significantly useful.
>
> Probably leave them in for now but deprecate them (maybe put in a
> deprecating comment).

I was thinking along the lines of having modules runing at different debuging levels and logging to different files. This could come in useful when debugging a module while others are known to work. We could do this by having a debuglevel and
debugfile entry in a default section, and new entries in sections that will have different values. But this requires changing the conf file every time a new debug level is wanted. I was thinking of a situation where one would be debugging a conf file and temporarily using higher debugging levels.


> > > 18: true, well spotted. My inclination would be to put a warning
> > > somewhere, maybe check each section name at load time.
>
> > Yes, its the easiest solution. No need to spend a lot of time with this.
>
> Done.
>
> > Like I said before, I don't really like the way we load everything into
> > memory. The linux cache buffer probably already does that. It actually
> > uses up any spare physical memory for cache buffers, so reading the conf
> > directly from file is practically as fast as loading it into virtual
> > memory.
>
> But it has to be parsed, which takes time...

Yes, but this is setup time. It doesn't introduce any delays during normal running of the PLC. In machines with a fast CPU and lots of memory, either option will work. I guess it comes to what is preferable in a small plc with a slow CPU and
less memory (no disc to swap virtual memory!):
- faster startup when switched on (important, for example, after a power failure ?) - or smaller memory footprint.

I remember you suggesting some time back that for small CPU and memories, we could pre-parse the config file. I guess this is the way to go.

For now we should just let it be. Maybe I'll get round to your point of view later on.

> > > 21: not just write points - that can be useful with a read-only point, too.
> > > (I'm not sure if I understand you though - what's the "id" for?)
>
> > Sorry, should have been char *name. i.e. the name of the point used in
> > plc_pt_by_name(char *name)
>
> > But with read points, who will then be the owner? Can't give every module
> > permission to make itself the owner of an alias.
>
> I was expecting that the alias would be into an already-existing point
> handle. So that you'd obtain a handle for a whole word, and then split it
> into smaller pieces using the alias function. (That was probably a
> misunderstanding on my part - I was going through it rather quickly.)
>
> In any case, the permission for points is specified by the SMM map in the
> linuxplc.conf file; if you ask for an alias for one of those points, then
> the same permissions apply. Whoever owns a point owns all the pieces.

OK. I get your drift. It also works for read points, as long as we only allow aliases of already existing points.

Actually maybe this isn't so important afterall. If other modules are going to access these aliases, then it is probably better to have them defined in the conf file where everybody can see them.


> > > 22: there are two good reasons why each point has a single owner:
> > > - firstly, for traceability. If a point changes, it should be easy
> > > to trace why, and having a single unambiguous owner allows that.
> > > - Secondly, the way the SMM library is implemented, every module
> > > writes *all* points it has write access to every time it calls
> > > plc_update(), and it typically calls plc_update() each cycle.
> > > This is fundamental to the current design of the library.
>
> > > If you need two modules to modify a PID flag, they should each modify an
> > > internal coil instead and then there should be an explicitly-made and
> > > explicitly-coded decision about how these are combined (usually AND or OR).
>
> > Yes, I agree with you here. But we still have the problem that we
> > currently support this. Consider the following linuxplc.conf:
>
> > point I0 "I0" owner1 at 0.0
> > point I1 "I1" owner2 at 0.0
>
> That's an error. It's not detected at the moment, but it's an error.
>
> Similarly, it's an error if two points partially overlap:
> point I0 "I0" owner1 at 0.0 2
> point I1 "I1" owner2 at 0.1
>

Agreed. This will greatly simplify the design. No need to complicate matters un-necessarily.
As soon as we define a policy for error messages, I can put these checks in.

> BTW, we should probably make the .0 optional when length is 32.

Good idea. And make the 32 bit length optional. So:

point I0 "I0" owner at 0.0
would have default length 1

point I1 "I1" owner at 1
would have default length 32

If you agree with this I can code this into the smm-mgr.c

Mario.

--
----------------------------------------------------------------------------
Mario J. R. de Sousa [email protected]
----------------------------------------------------------------------------


_______________________________________________
LinuxPLC mailing list
[email protected]g
http://linuxplc.org/mailman/listinfo/linuxplc

 

Jiri Baum:
> > OK, no worries. Though I'd have thought an error in the config would
> > warrant an error message rather than just a warning (commented out)...

Mario de Sousa:
> I had not yet written the log file library when I did this change. Never
> got around to uncommenting the warning log...

OK, no worries, as long as we don't forget...

> > (Hmm, that probably points to a deeper problem: is there policy for
> > error and warning messages?)

> Yes. When I finished the logging library I mailed the list sugesting a
> discussion should be made to come up with a policy of when and how to log
> messages. It never started. Maybe we can start it now?

Perhaps it wasn't the best way of phrasing it

(It often happens that the best way to start a discussion on a topic is to present a proposal. If no-one responds, you simply use the proposal as the solution...)

Actually, in a way, you already have made a proposal by coding two levels into the error-handling system.

> > > > 16: probably not. Same with PLClogfile and PLCdebug (though those
> > > > perhaps have more justification).

> > > I'm having a change of heart. Leave them as possible overides of what
> > > is defined in the conf file. What do you think? Maybe just let them
> > > be until somebody complains...

> > I'm not sure. In some ways having overrides for some values but not for
> > others is confusing (because inconsistent). If we want overrides, we
> > should probably have a general syntax, --section:variable=value or
> > something, but I can't see how that'd be significantly useful.

> > Probably leave them in for now but deprecate them (maybe put in a
> > deprecating comment).

> I was thinking along the lines of having modules runing at different
> debuging levels and logging to different files. This could come in useful
> when debugging a module while others are known to work. We could do this
> by having a debuglevel and debugfile entry in a default section, and new
> entries in sections that will have different values. But this requires
> changing the conf file every time a new debug level is wanted.

That's not a big problem - simply make a three-line config that contains just the new debug levels and a *include of the main config. After all, you may be experimenting with other things in the config as you debug.

Once we have a master-module putting up and taking down all the other modules, any cmd-line options to be passed will be in the config anyway.

(The two command-line options that are needed are the config file to use and the module name. Module name can't be hard-coded because two instances of the same code might be running simultaneously.)

> I was thinking of a situation where one would be debugging a conf file
> and temporarily using higher debugging levels.

Yes, from that point of view it makes sense to have debug levels on the command line. Probably leave them there, they aren't doing any harm.

...
> > > Like I said before, I don't really like the way we load everything
> > > into memory. The linux cache buffer probably already does that. It
> > > actually uses up any spare physical memory for cache buffers, so
> > > reading the conf directly from file is practically as fast as loading
> > > it into virtual memory.

> > But it has to be parsed, which takes time...

> Yes, but this is setup time.
...
> I remember you sugesting some time back that for small CPU and memories,
> we could pre-parse the config file. I guess this is the way to go.

That's right, I think I did suggest that.

That'll be the best of both worlds - fast startup, low memory overhead (mmap(2)ped file) and as a bonus we can leave out the parsing code.
Disadvantage that you can't edit it directly, but on a small machine you can't do that anyway.

> For now we should just let it be. Maybe I'll get round to your point of
> view later on.

> > > > 21: not just write points - that can be useful with a read-only
> > > > point, too. (I'm not sure if I understand you though - what's the
> > > > "id" for?)

> > > Sorry, should have been char *name. i.e. the name of the point used
> > > in plc_pt_by_name(char *name)

> > > But with read points, who will then be the owner? Can't give every
> > > module permission to make itself the owner of an alias.

> > I was expecting that the alias would be into an already-existing point
> > handle. So that you'd obtain a handle for a whole word, and then split
> > it into smaller pieces using the alias function. (That was probably a
> > misunderstanding on my part - I was going through it rather quickly.)

> > In any case, the permission for points is specified by the SMM map in the
> > linuxplc.conf file; if you ask for an alias for one of those points, then
> > the same permissions apply. Whoever owns a point owns all the pieces.

> OK. I get your drift. It also works for read points, as long as we only
> allow aliases of already existing points.

> Actually maybe this isn't so important afterall. If other modules are
> going to access these aliases, then it is probably better to have them
> defined in the conf file where everybody can see them.

Yes, that'd probably be better.

(Though anonymous aliases - ones without names - are simple enough that we should allow them. That is, a function which given a point handle and an
offset/length within that point will return a point handle just to that piece.)

We might even have pre-defined point types, so that you say
[SMM]
PID p0 "pid-controller 0" ownerA ownerB ownerC
and it effectively defines a bunch of points, some owned by ownerA, some by ownerB and some by ownerC (though the input and output points themselves should probably be defined separately, so that they have sensible names).

---

> > > Yes, I agree with you here. But we still have the problem that we
> > > currently support this. Consider the following linuxplc.conf:

> > > point I0 "I0" owner1 at 0.0
> > > point I1 "I1" owner2 at 0.0

> > That's an error. It's not detected at the moment, but it's an error.

> > Similarly, it's an error if two points partially overlap:
> > point I0 "I0" owner1 at 0.0 2
> > point I1 "I1" owner2 at 0.1

> Agreed. This will greatly simplify the design. No need to complicate
> matters un-necessarily.

OK.

> As soon as we define a policy for error messages, I can put these checks
> in.

OK. Sorry about not saying that somewhere - I've been assuming all along
that points are distinct, but I guess I never said it.

> > BTW, we should probably make the .0 optional when length is 32.

> Good idea. And make the 32 bit length optional. So:

> point I0 "I0" owner at 0.0
> would have default length 1

> point I1 "I1" owner at 1
> would have default length 32

> If you agree with this I can code this into the smm-mgr.c

Sounds good.

Another thing that would be nice would be to make the "at x[.y]" optional, too (that's why the word "at" was there to begin with).
point I0 "input 0" owner
would allocate the next available bit, and
point R1 "register 1" owner 32
would allocate the next 32-bit word (aligned at the next word boundary).

(The algorithm may or may not like to go back and fill in any bits skipped over for word-alignment reasons - doesn't matter, as long as it's
consistent.)

Rationale: the user shouldn't need to care about bit addresses in the shared memory map. (Also, a future version may wish to optimize them somehow and it can't do that if they're user-specified.)


Jiri
--
Jiri Baum <[email protected]>
"Coffee machine out of paper" (error message missing from Coffee mini-HOWTO)

_______________________________________________
LinuxPLC mailing list
[email protected]
http://linuxplc.org/mailman/listinfo/linuxplc