Made globals into Class properties

[antonvh]

Lib wouldn't compile unless I did this.

[ddemidov]

This makes file cache local to each instance of device class, which defies the purpose of file cache.

[rhempel]

I got stuck last night trying to get Bluetooth PAN working reliably, hopefully I'll have some time to look at this in the afternoon. The file cache, as @ddemidov points out, should be a class property.

Arguably, the files backing the attributes of each device can be cached per device class instance.

The downside of this is it's harder to manage expiring cached handles because they are not all in one place :-) I don't have this implemented yet because I have not run out of file handles for a single process, but that's a settable kernel limit, so we need a plan for when we hit it.

[antonvh]

What is the point of caching exactly? The filesystem is only parsed once, at the instantiation of a Device, which is not a time critical process. Or am I overlooking something?

[ddemidov]

See ev3dev/ev3dev-lang#12

[antonvh]

I see... And a per-device cache uses up too much memory? Are files much shared between devices? If not, it shouldn't have too much impact on memory and lead to cleaner code.

[ddemidov]

Per-device cache should work in principle, since each device reads its own node in sysfs. As @rhempel said, it would make things harder to manage in the future, when we want to e.g. limit number of open filehandles. But there is another problem: __EV3_MODULE_connected is a global dictionary that is used to skip devices that were already connected. The patch would definitely break this functionality, although I think it is somewhat broken anyway.

EDIT: It seems that __EV3_MODULE_connected is no longer used like this. In fact, its not used at all.

By the way, what did you mean by "lib would not compile without this"? What was the exact error?

[antonvh]

So I can remove __EV3_MODULE_connected from the code?

(The error is NameError: global name '_Device__EV3_MODULE_filehandle_cache' is not defined, btw.)

Too much files open... isn't that the problem of the user of the library? The user can always ulimit -n 65536 Or limit the number of devices in use in his script?

[antonvh]

I'd even say you make the problem worse, as you skip python's built in garbage collection when you use globals.

[antonvh]

It keeps nagging at me. Another clean solution would be to have a per-device limit of open files. We could make that a smart limit: keep critical files like 'position' open and close less critical files like 'commands'.

[ddemidov]

You could also move file cache into Device scope. In C++ this would be a static member. For example, the following snippet outputs {42: 2, 101: 1}:

class A:
    d = {}

    def __init__(self, i):
        if i in A.d: A.d[i] += 1
        else: A.d[i] = 1

a = A(42)
b = A(101)
c = A(42)

print(A.d)

[rhempel]

Believe me, it nags on me as well. @antonvh is correct, in most cases only a few of the files would need to be read or written frequently. We can handle this a couple of ways.

1. One would be a limit on the number of cached handles per device instance, with a simple timestamp to close and dump the oldest ones.
2. On the other hand, a "priority" flag could be added to the spec so that the autogen system could generate code to flag an attribute's file handle as a candidate for caching.
   I think I prefer option 1 - and another advantage of per-device handles is that we can close them up as part of the cleanup function when the device gets collected.

[antonvh]

That is exactly what I did in the pull request. I didn't do the smart cache yet, but file cache is in the device scope now, so that intelligence can easily be added later. Python garbage collection will clean and close files automaticilly at the end of script. If you want it earlier you can simply do:

m = Motor(name="motor0")
del(m)

[rhempel]

@antonvh, I merged in quite a few of your changes, but have not tested them. The work you did to fix up the getters and setters I implemented in the autogen templates. The specialized motor api additions I think are better dome in a separate helper module - I would like to keep the bindings as simple as possible.
Thanks for the code suggestions, and you have a credit line in the header now :-)
Do you want some ev3dev.org tiles?