<wpwrak>
(evb) hmm, no documentation. and no mention of design files.
<wpwrak>
(mech) excellent ! that helps :)
<wpwrak>
(evb) it's actually even a bit more expensive that i had thought. but okay, EVBs prices are more or less arbitrary.
<drizzt>
yep
<drizzt>
evb documentation is listed under Feature -> software
<drizzt>
AM389x and C6A816x Software Development kits provided via included 8GB SD Card
<drizzt>
Documentation
<drizzt>
8168 EVM Quick Start Guide
<drizzt>
8168 EVM Hardware Users Guide (SD card)
<drizzt>
...
<wpwrak>
that's just a user's manual
<wpwrak>
not schematics, layout, etc.
<drizzt>
hum.. I do not know how they got them, but we are actually working on a Davinci processor (TI) with a evb from spectrum digital, and i have all this stuff
<drizzt>
(and no direct NDA with TI or spectrum digital)
<wpwrak>
okay :)
<wpwrak>
schematics are usually included with evbs. that's why i was surprised they didn't mention it. layouts usually not, though.
<wpwrak>
but what do you actually expect to need all this processing power for ? most of the time, this device should do pretty much nothing. maybe chat with the sensors once a minute. process an event or two. the big moment would be then the user presses a soft-button. then you'd have to animate it. a sinclair zx81 could probably do it ;-)
<drizzt>
it's even more expensive than the new one !
<wpwrak>
whoopie ! must be very low volume :)
<drizzt>
wpwrak: that for a use limited to domotics control
<drizzt>
(wich could be done by anything, in fact)
<wpwrak>
for watching tv, you have a big screen elsewhere :)
<drizzt>
but the goal is to provide a tablet as well
<drizzt>
for anything the community would think of
<wpwrak>
hmm, how would that work ? i thought this device was the domotics controller ? so it may have cables connecting to power line/comms network. not very convenient for a tablet.
<drizzt>
we already have a few other aplications for it in mind
<drizzt>
yep, depends on the intended use
<drizzt>
there are many many use cases
<wpwrak>
but a the others would be completely different classes of devices
<drizzt>
some allowing for wired protocols, some not
<wpwrak>
if you want to compete with the iPad, that's a different story :)
<drizzt>
lol
<drizzt>
not compete with, but yes, that's part of the idea
<wpwrak>
sigh. whatever apple does, one year later, everyone wants to do it too.
<wpwrak>
in a way, that's nice, though. so all the competition crowds in that year's fashionable corner, fights their patente wars, etc., and leaves plenty of niches for the rest of us :)
<wpwrak>
i like wolfgnag's approach for the ben. take things that are simple. fully commodized. large parts of the design proven and r&d already written off. that way, you can focus your resources on what makes you different, not waste them in the struggle to stay at the bleeding edge.
<wpwrak>
also leaves more room to play with the really outlandish ideas, like milkymist ;-)
<wpwrak>
anyway, let's see what he thinks about that tablet
<wpwrak>
rafa: btw, will you also upload the respective source packages ?
<wpwrak>
rafa: (makes things much easier for gpl compliance and such)
<rafa>
wpwrak: just for extra packages.. (few ones < 10) The rest is the repo built from OE without modifications. If some user wants to get the source he could read the metadata of the package which has a "Source : http://..."Â Â field.
<rafa>
get the source=get the source of a specific package
<wpwrak>
ah, i see. i guess that's alright.
<rafa>
wpwrak: but if we need to host every source code I can upload them :) .. The OE building thing used 100GB+ and a bit part of that were the sources
<rafa>
bit=big!
<wpwrak>
disks are cheap ;-))
<rafa>
wpwrak: for example.. if you check the metadata of bash package downloaded from jlime or soon from qi :
<rafa>
wpwrak: but I am okey with both things.. if we need to uploaded the sources then I will
<rafa>
wpwrak: about disks are cheap : http://www.happypenguin.org/ (it is the web site of the company making/selling propietary linux games)
<rafa>
company=linux game publishing company
<rafa>
"The Western Digital Caviar Green 2TB drive that failed has chemical degredation on the surface making the data recovery much slower and harder"
<wpwrak>
rafa: nice :) and despite having a backup system
<rafa>
wpwrak: I was thinking to talk with them to ask if they need people/developers (they use a lot sdl and opengl for everything).. No now it seems :P
<wpwrak>
right now they need data restoration experts :)
<wolfspraul>
rafa: I added your pubkey to the www-data and root accounts on turandot.qi-hardware.com
<wolfspraul>
if you are not sure what you are doing, I suggest to use the www-data account first
<wolfspraul>
you can definitely copy files into the downloads.qi-hardware.com root folder with it
<wolfspraul>
/srv/www/downloads.qi-hardware.com in the local filesystem
<rafa>
wolfspraul: which will be the final link?.. for example.. if, for example, I make a directory named "jlime" (/srv/www/downloads.qi-hardware.com/jlime/). Will it be http://downloads.qi-hardware.com/jlime/ for web browsers ?
<rafa>
wolfspraul: BTW, thanks for the info and pubkey work
<wolfspraul>
yes I think so
<wolfspraul>
the problem is that we have a similar downloads.qi-hardware.com/openwrt url, but that is for sources only
<wolfspraul>
I have no problem with that, so if it doesn't bother you then just use a /jlime folder
<wolfspraul>
then /openwrt will be sources, but /jlime will be binaries
<rafa>
wolfspraul: ah.. great.. Let me work and I will tell you if I feel confident that all is okey (rights, copy, etc).
<wolfspraul>
rafa: ah wait. wrong!
<wolfspraul>
the openwrt source location is just called /sources
<wolfspraul>
downloads.qi-hardware.com/sources
<rafa>
wolfspraul: for me is okey.. If you have a better idea for names please tell me :)
<wolfspraul>
well, just make a /jlime and put whatever you like in there
<wolfspraul>
no I don't
<rafa>
wolfspraul: okey, I will
<wolfspraul>
/jlime is good
<wolfspraul>
just please keep all mp3 and h.264/mpeg4 stuff out
<wolfspraul>
that's my only concern :-)
<rafa>
yes.. /jlime is okey for me as well. so we can have /jlime/images/ and /jlime/repository/ for example.
<rafa>
wolfspraul: Yes, I am doing my best about shit packages
<rafa>
wpwrak: wolfspraul : tuxbrain_away : I am going to check the files that every package contains.. trying to find if some package has mp3 or mp4 inside, or similar. I think that because it is a repo built from OE there is not packages with those file formats
<rafa>
but I want to be sure
<wolfspraul>
great
<wpwrak>
rafa: excellent, thanks !
<wpwrak>
btw, what is "db" ?
<wpwrak>
sounds like the berkeley database, but that wouldn't be a problem package
<rafa>
wpwrak: yes, that is berkely database.. and I did not remove that finally ;) I checked every package, why it is there in the README file yet :)
<rafa>
wpwrak: my grep found that because this is a Oracle product
<rafa>
why=I do not why
<wpwrak>
aah :) well, oracle per se isn't evil ... even if they lately try hard to look that way :)
<rafa>
wpwrak: the idea was to find if there is a "Oracle's java"
<rafa>
only
<wpwrak>
aah, i see
<rafa>
wpwrak: I did two lists: one list to remove the mpeg* technologies. And another to find forbidden items in Fedora.
<wolfspraul>
drizzt: are you there?
<wolfspraul>
you said you have a meeting today, but I would like to postpone my answer until tomorrow, if possible
<wolfspraul>
also I have a question for you: what kind of products did you outsource/build in this style before? starting with a reference design, and then doing everything else your own/based on your directives?
<wolfspraul>
were your partners based in France or other countries?
<kyak>
xiangfu_: are you there?
<xiangfu_>
kyak: yes
<kyak>
xiangfu_: abook in openwrt-packages is marked as broken because it needs at least 70x20 terminal.. What do you think if i set it dependent on setfont2 and start abook via wrapper that would first set the 4x6 terminal font which provides even more than 70x20?
<xiangfu_>
kyak: hmm... how about when abook exit. can it set the font back?
<kyak>
hm i think i can do it
<kyak>
setfont .. && abook && setfont ..
<kyak>
the last setfont won't start until abook exists
<kyak>
*exits
<kyak>
(in fact, we can even patch the abook to setfont on start and unset it on exit :)
<kyak>
got to go now
<rafa>
wpwrak: fffff.. a package which the source is from maemo has a mp3 inside.. pff
<rafa>
wpwrak: you mean to look for that into every package?
<xiangfu_>
kyak: the abook compile fine and it can start without change font to 4x6 right? (only cann't edit)
<wpwrak>
rafa: well, if they're that good at hiding the thing :) but it's probably enough if you scan the files names for MP3 or mp3.
<wpwrak>
rafa: getting rid of this junk really is harder than it seems :-(
<rafa>
wpwrak: I did :) I have a text file with the list of files of every package on the repo.. so I can look for file names ;)
<wpwrak>
ah, cool :)
<rafa>
wpwrak: I did egrep -i "..mp3$|avi$.. etc.." just that package looks no nice
<rafa>
wpwrak: but well, there could be some hidden stuff which is not easy to find as you said :(
<kyak>
xiangfu_: yeah, it doesn't seem to give any warning messages.. but it also doesn't fit to the screen (compare with what you see via ssh or at your PC)
<rafa>
wpwrak: I tested the www-data user under /srv/www/downloads.qi-hardware.com/ as wolfgang suggested and all is okey. So we have all we need to upload stuff ;)
<xiangfu_>
kyak: I found if we use "setfont2 ...4x6" in ssh terminal. the nanonote's console will changed.
<xiangfu_>
kyak: I am testing this for if someone run the "setfont .. && abook && setfont .." in ssh
<xiangfu_>
kyak: so I think let's just wrap the abook. and add it to config.full_system :)
<kyak>
xiangfu_: good. let's do it :) give me some time
<wpwrak>
rafa: (www-data) great ! some day, wolfgang will have to set up real user accounts there. it's a bit messy if everyone shares the same account ...
<drizzt>
we will have a partnership with my university (CPE-Lyon), and one of the professors pointed me this
<drizzt>
I haven't looked at it yet
<drizzt>
just at this page
<drizzt>
but :
<drizzt>
Board Revisions & Documentation ... Gerber File ... Schematics ...  Bill of Materials (BOM) ... Allegro Design File ...
<drizzt>
sounds quite good
<wpwrak>
i see that you're aligning your requirements with reality. that's good :-)
<wpwrak>
allegro isn't so nice, though. a proprietary tool.
<drizzt>
yep
<drizzt>
I'd rather have kicad
<drizzt>
do you know if there's a tool to convert between any of the existing CAD formats ?
<drizzt>
or if some of the tools can save files in a usable format
<wpwrak>
pheeew ... there are some tools to convert some things. but i'm not sure how usable they are. i've seen a lot of footprint conversions. but then, if you convert from a proprietary system, there's a good change that these libraries will be non-free too.
<wpwrak>
so it may just be more efficient to redo everything manually and make sure you have a good review process.
<wpwrak>
drawing schematics symbols and footprints isn't too hard for kicad (if you use fped for the footprints. the footprint editor kicad comes with isn't really usable)
<wpwrak>
for the layout, you could actually reverse the gerbers. but i'm not sure if this catches all the details. probably not.
<wpwrak>
(reverse the gerbers) kicad can load a gerber file and convert it into a kicad board file
<drizzt>
ok
<drizzt>
so
<drizzt>
PCI-Express and Sata were kind of "why not, use them if they are here"
<drizzt>
so this design is rather nice as a starting point
<wpwrak>
yeah, looks nice. if you can make your device just use the existing board, that would be even easier. that would remove a LOT of work.
<drizzt>
ethernet over USB is not the best, but the goal is not to build a high bandwidth web server :)
<drizzt>
yes
<drizzt>
the Ethernet + 2 USB connector is a bit too big
<wpwrak>
qbject: more or less in the geographic middle of it :)
<qbject>
Ahh.
<qbject>
That's cool.
<qbject>
Just asking because I sometimes fantasize about emigrating to Montevideo.
<wpwrak>
oh, why montevideo ? kinda boring place
<wpwrak>
well, better government, i think. not that their argentine counterparts would make that bit particularly difficult :)
<drizzt>
wpwrak: Buenos Aires ?
<qbject>
I like meat and freedom. It sounds cheap, compared to places in the US. And I don't need fun, just a lathe, milling machine, and net connection.
<wpwrak>
drizzt: yup
<wpwrak>
qbject: yup, argentina is cheap. uruguay probably too. you can get mills and such here too. although imported stuff tends to be more expensive than in the us. about 150-200% of what you'd pay there. for consumer electronics, often even more.
<drizzt>
ouch
<qbject>
wpwrak: fair enough. Ideally I'd acquire the equipment here then ship it down. Not sure how tricky the port authorities are to deal with. Then I could BUILD consumer electronics.
<drizzt>
wpwrak: why are you living there ? any particular reason ?
<wpwrak>
shipping stuff when you're moving might work. otherwise, it gets very tricky.
<drizzt>
(maybe I already asked some times ago on gta02-core)
<qbject>
wpwrak: that's probably how I'd work it. Get a container, move it all in one go.
<wpwrak>
drizzt: because i like it here :) when i finished my phd, i considered the choices: europe - been there all my life, can go back anytime i want, not much of a challenge. switzerland - been there all my life, getting boring. us - been there for conferences and such, didn't find it too attractive. south america - been there for some conferences and vacations. like it :)
<drizzt>
ok, by personnal choice then :)
<wpwrak>
drizzt: yup. for kernel hacking, internet and a pc is all you really need :) i got into playing with hardware later
<drizzt>
a PC, but with two screens :)
<drizzt>
which you can have anywhere :)
<drizzt>
and playing with hardware should be no harder there :)
<drizzt>
is it you who found the SMT line in a university for gta02-core ?
<drizzt>
wpwrak: another question, completly independent from all others
<wpwrak>
drizzt: (pc) three screens, actually :) it's a bit of a challenge to get this to work ...
<drizzt>
what's your experience in kernel hacking ?
<wpwrak>
drizzt: (university) that was jon "maddog" hall who "found" the smt line
<drizzt>
er, not so much of a challenge nowdays, I sometimes have up to five, depending on the place on my desk and what I have at hand (from mimo740 to 24" screens)
<drizzt>
using synergy makes it easier
<wpwrak>
drizzt: of the stuff that's in general use today: msdos-fs, initrd, linux-atm, diffserv, some other pieces here and there. did some projects in networking, file system, and of course embedded (linux on the psion s5, later openmoko)
<drizzt>
I often use my laptop as a third screen
<drizzt>
KO,
<drizzt>
OK
<drizzt>
would you perform code reviews ? (paid, but I'll need a bill)
<drizzt>
I'm also doing kernel hacking, and I'm writting my first driver "from scratch", so an external look would be nice
<wpwrak>
(reviews) in what area of the kernel ?
<wpwrak>
ah, first driver. always fun :)
<drizzt>
I already asked Greg KH, but his current position prevents him from doing it
<drizzt>
I already did modifications for specific things, but never designed from scratch
<wpwrak>
another frequent reviewer is christoph hellwig
<wpwrak>
what sort of driver ?
<drizzt>
thats a driver handling communication with a 8051 microcontroller over serial
<wpwrak>
UART ? or some other kind of serial ?
<drizzt>
most of it should have gone in userspace, but a few requirements and the fun of it made me do it inside of the kernel
<drizzt>
pure UART
<wpwrak>
uart is one of the suckier interfaces ...
<wpwrak>
(user space) indeed, the art is avoid putting stuff into the kernel ;-)
<drizzt>
yes
<drizzt>
but I need to handle an interrupt, and reply in less than 50 ms when this interrupt fires
<wpwrak>
interrupt coming over some separate line ?
<wpwrak>
50 ms seems more than generous enough for user space.
<drizzt>
but this simple part would have been possible standalone, with some protection, but I must not send the reply in the middle of a message, and not even send it if there's a message being sent
<drizzt>
yep, quite generous with no system load, but the system will handle video, and the processor is not that good
<wpwrak>
and who sends the messages ? do of course have all sorts of locks in user space as well ...
<wpwrak>
(no system load) that's what real-time priorities are for ;-)
<drizzt>
interrupt over a separate line, yes (gpio)
<drizzt>
yes, there's a lot of ways to do it in userspace, but as I said, it was for the fun of doing it
<wpwrak>
interrupts in user space are a problem, though. i wonder if there's still the little political problem that prevents us from having a general mechanism for them.
<drizzt>
got a state machine in high priority, and a Qt interface in low prio
<wpwrak>
alright
<drizzt>
and for the interrupt in userspace, I solved it on a VME board by providing a mechanism for userspace programms to register a signal that the kernel triggers when the interrupt fires
<wpwrak>
yup. but that's not something that's in mainline, is it ?
<drizzt>
with some interresting stuff depending on the signal (if I remember, RT signals can stack, thus simulating multiple interrputs)
<wpwrak>
the political problem being that this would make it possible for a large class of closed-source drivers to be written for user space
<drizzt>
no, it was a demo VME driver
<drizzt>
when I was employee, and not as much attached to free software as I am now
<drizzt>
I was following the guidlines of my employer
<wpwrak>
hehe :)
<wpwrak>
yeah, that happens. i also spent some time at ibm :)
<drizzt>
(one of the reasons that made me resign, and become my own boss :)
<wpwrak>
yup, freedom is good to have
<drizzt>
so, can I forward you the email for the review ?
<wpwrak>
i can have at least a quick look ...
<drizzt>
ok, I forwarded the original mail
<wpwrak>
better to avoid lines > 80 characters
<wpwrak>
see also linux-*/Documentation/CodingStyle, chapter 2, 2nd paragraph :)
<drizzt>
yes, I know, but my screen are much wider than they are high
<wpwrak>
the terminals of the readers of your code may not be, though
<drizzt>
do not take care about anything marked /* debug */
<drizzt>
or lines starting with "/* */"
<wpwrak>
line 1805 s/if(/if (/
<drizzt>
those have been added afterwards to allow the author of the 8051 firmware to debug his code
<drizzt>
yep, thanks
<wpwrak>
hmm, is the CRC different from the one in lib/crc-ccitt.c ?
<drizzt>
mind that the conditions in the mail still apply
<drizzt>
from my tests yes
<wpwrak>
ah, conditions ... you mail is kinda hard to read too, with all the oversized lines ...
<drizzt>
maybe just a question of initial value, but I tried a lot of tools on the net before getting one wich gave me the same CRC as the 8051
<wpwrak>
crcs can be incredibly tricky, yes
<drizzt>
and I'm not so good with these kind of stuff
<drizzt>
so when I found one which matched, and which gave C code for the implementation ...
<drizzt>
I can pay for two hours for you and two hours for another kernel developer which would be
<drizzt>
interrested, at a rate of 110 euro per hour (+ taxes) (total of 440 euros without taxes).
<drizzt>
so two hours for you :)
<xiangfu>
wpwrak: Hi I got one email "fped_0.0+r5986-1_amd64.changes ACCEPTED into unstable"Â Â :)
<xiangfu>
wpwrak: forwarding to list
<wpwrak>
xiangfu: congratulations ! now wolfgang owes you a crate of champagne  ;-)
<wpwrak>
drizzt: kewl. the EI_KEY_... stuff wants to be an enum
<wpwrak>
linux/embedded_interface.h is missing
<drizzt>
yes, it is in the userspace code, and here it will be removed
<drizzt>
the driver does not care about what the key means
<drizzt>
(they are all unused)
<wpwrak>
i see
<drizzt>
same for EI_LED_*
<wpwrak>
may be better if you do the clean up you've already planned first ? not much use to review stuff that'll go away anyway. that is, unless you want to compare coverage ;-)
<drizzt>
I had not much time to do so when I sent the first request
<wpwrak>
you have a few indentations made up of spaces. if using vi, you can find them with /^__ (where _ would be a space)
<drizzt>
the author of the 8051 firmware wanted to review my code because he said that my handling of the protocol was wrong and generated the problems I reported, such as 8051 resets, messages getting lost, crc errors, ...
<wpwrak>
heh :)
<drizzt>
so I also asked for a review from someone used to kernel drivers
<drizzt>
but during the meeting with the custommer he finaly left behind the idea of reviewing my code
<wpwrak>
first pass is always use of whitespace :)
<drizzt>
whatever mistake I could have made, the 8051 should not reset itself ...
<drizzt>
so he decided to review his code first
<wpwrak>
unless it's spec'ed to reset when confused
<drizzt>
leading spaces corrected (i love vi :)
<drizzt>
not at all, rather the contrary
<wpwrak>
more whitespace, e.g., line 425: it should be  struct message *free  not  struct message* free  (CS 3.2, 4)
<wpwrak>
you also occasionally don't put a blank line between block-local declarations and statements, e.g., in emb_intf_put_free_message
<drizzt>
the 8051 should be the "security" part, cutting everything off in case of problem
<wpwrak>
global declarations between functions aren't so nice either (partial)
<drizzt>
it's a medical device (not a critical one, you can keep going to the hospital)
<wpwrak>
(safety-critical) ah yes, then a reset every now and then isn't so good :)
<drizzt>
(message *free) : this is a "bad habbit" of mine
<drizzt>
in fact, moving the '*' from "message *free" to "message* free" made me understand pointers
<wpwrak>
(bad habit) yup. i see it at a few places :)
<drizzt>
I would say everywhere
<wpwrak>
btw, you can save space by not indenting the "case" in a switch, see CS 3, par 2
<drizzt>
what's strange is that every time someone has difficulties with pointers, explaining with the '*' next to the type and not the name makes them understand (along with a short explanation)
<wpwrak>
hehe, that makes sense
<wpwrak>
for commenting struct members, it improves readability dramatically if you align the comments
<drizzt>
(struct message*) free  <-- free is a (struct message*)  <-- free is a pointer to something of the type (struct message)
<drizzt>
and then (*free) becomes obviously ("the something of the type (struct message)
<drizzt>
this is what had me understand pointers, and allowed me to explain to many other students
<drizzt>
I sometimes wonder if the professor was that good though ...
<wpwrak>
regarding the CRC, did you check if the one in lib/crc-ccitt.c is really different ? i see that you have a funny initial value.
<drizzt>
(crc) yes, I know it may be only a matter of initial value
<drizzt>
I did not chek inside of kernel, but I put the kernel version in a userspace test programm, and got different values
<drizzt>
but at that time I did not try to change the initial value
<drizzt>
I had no knowledges about CRC (and I still have very few)
<wpwrak>
okay, so you looked at that one. good. i wonder where the initial value comes from. also, it's a pity the polynom isn't specified.
<wpwrak>
whenever i've figured out crcs enough for them to make sense, i have to drink a lot afterwards, to forget all this again as quickly as possible ;-)
<drizzt>
and as 1D0F was mentionned in the documentation of the 8051 programm, i gave it a try
<wpwrak>
aah ! well, your algorithm swaps the bytes. so it may be 0x0f1d.
<drizzt>
maybe that's why I never managed to used those which proposed to set the initial value, as I should have tried 0x0f1d and not 0x1d0f
<wpwrak>
the table looks different too, though. not sure what to make of this.
<drizzt>
the table is generated in the example found on the website
<drizzt>
I printed it to prevent calculation upon startup
<drizzt>
(printed and put it in my code)
<wpwrak>
yup, that's common practice. nothing wrong with that.
<wpwrak>
just having your own private implementation of an apparently "standard" CRC in a driver is something people will complain about when you submit this for mainline inclusion
<drizzt>
hum .. I can send this for mainline, but I do not see the point, it's related only to the protocol on a single product ...
<wpwrak>
you seem to have a number of code patterns in "struct embeded_intf", such as flags_* and key_*, also with very long names. could it be cleaner to introduce a "struct ei_queue" or such for these ?
<wpwrak>
ah, i see. thought you were interested in getting it into shape for mainline submission
<drizzt>
it will be in the sources provided with the product (I'll put it on the internal system SD :)
<drizzt>
I wanted to know if it would be OK for mainline submission
<wpwrak>
drizzt: you could simplify things by making curr_msg_id an uint8_t
<drizzt>
I can send it, as an exemple, but then it may be much more usefull on the web, outside the kernel, with the protocol and some explanations
<drizzt>
though I think the protocol is under NDA .... :(
<drizzt>
but it's quite easy to find from the sources :)
<drizzt>
and it's such a bad thing that it's better not to make it public
<wpwrak>
yeah, sources tend to give such things away :)
<wpwrak>
the messages do look a little strange indeed
<drizzt>
(the value 100 is sent as the string "100", not as 0x64)
<drizzt>
so it's 3 bytes on wire, not one
<drizzt>
and then, some messages have been split in three, to keep them short
<drizzt>
they told me it was for "readability"
<drizzt>
a human can read it, and write it very easily ..
<drizzt>
but in fact, this is so much wrong .. I have some difficulties with the CRC calculation without a programm ...
<drizzt>
so all the protocol should be rewritten .. but won't ever.
<drizzt>
yep for the paterns
<drizzt>
flags, but also wait queues
<drizzt>
hum, your version does not have the wait_queues
<wpwrak>
haven't gotten to the wait queues yet. btw, minor nit: you could strip one layer of parentheses in lines 516 and 525
<wolfspraul>
wpwrak: fped is in Debian unstable :-)
<wolfspraul>
so is xburst-tools
<drizzt>
had to intriduce them to put userspace programm to sleep while waiting for the replys
<wpwrak>
wolfspraul: world dominaton just got a little closer :-)
<wpwrak>
drizzt: why, busy-waiting is so much more impressive ;-)
<drizzt>
lol
<wpwrak>
drizzt: also, if (...) { single-statement; }Â Â doesn't need the curly braces. again, some space saved :)
<drizzt>
specially when the doctor is looking at the video
<wpwrak>
drizzt: the 'b' case in emb_intf_handle_reply could go to a separate function. this also reduces the indentation level
<drizzt>
hum, i stick to these since i often add debug and forget to add the curly braces, then wonder what's going on ....
<drizzt>
right for 'b' case :)
<wpwrak>
'c' case etc., the memcpys look unwieldy and they probably aren't portable either. how about emb_intf.voltages.val_5v0 = read_param(reply, 0);Â Â with a suitable read_param function ?
<drizzt>
and the switch becomes readable :)
<wpwrak>
'f' and 'g' also look as if they could be simplified
<drizzt>
yes
<drizzt>
h too (same simplifacation in fact)
<wpwrak>
drizzt: emb_intf_handle_received_message: if (!(msg->type & 0xf000)) { .... and then if (!(msg->type & 0xf000)) { again ?
<wpwrak>
and again, this HUGE if should be a separate function
<wpwrak>
and you could make emb_intf_handle_received_message a wrapper. spin_lock_irqsave(); if () __emb_intf_handle_received_message(); spin_unlock_irqsave();
<wpwrak>
then you don't need to worry abou the lock inside __emb_intf_handle_received_message() and the code gets a lot easier to review
<wpwrak>
ah wait .. you unlock it again inside. sigh.
<drizzt>
and there's other place where I need such things because of code that may sleep
<drizzt>
emb_intf_put_free_message
<drizzt>
(not sleep here, but lock too)
<wpwrak>
i see the lock-list_empty-del-unlock idiom quite a number of times. may be worth putting this into some helper function
<wpwrak>
see also CS 6: "Functions should be short and sweet, and do just one thing.  They should fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24, as we all know), and do one thing and do that well."
<wpwrak>
perhaps the single most important rule ;-)
<drizzt>
yes, I should cut in many more functions
<wpwrak>
this often also helps to spot a pattern that leads for further simplificaiton
<drizzt>
they tend to grow and I do not split them enought
<wpwrak>
it's like peeling an onion. with each layer of complexity you remove, the layer below becomes more visible
<wpwrak>
eek. emb_intf_handle_cmd_set: case doesn't need curly braces
<wpwrak>
the mask in ((val >> 16) & 0xffff is redundant if val is an uint32_t
<wpwrak>
drizzt: i also see that your faith in operator precedence is weakening in this function. there's a lot of parentheses you don't need
<wpwrak>
a lot of the message generation looks like stuff that could live in user space ?
<drizzt>
yes
<drizzt>
(the whole driver should)
<drizzt>
but this way the user space does not care about the protocol
<drizzt>
else there would be parts in the kernel and parts in the user-space
<drizzt>
and it's an "arrangment" with the person who does the user-space tool
<drizzt>
so he just have to use a few ioctl
<drizzt>
he wanted to kill the man who wrote the protocol when he read it
<wpwrak>
hmm. i wonder if the whole locking logic is really right. e.g., emb_intf_data_process -> emb_intf_handle_received_message -> emb_intf_handle_reply can sleep. and so can emb_intf_handle_cmd_get. yet you're using a spinlick for key_lock
<wpwrak>
spinlOck
<wpwrak>
or wait .. not sure about it being able to sleep ... lemme check ..
<wpwrak>
yes, it can. there's a "down" there :)
<drizzt>
key_lock ?
<wpwrak>
emb_intf.key_lock
<drizzt>
yep, but there's no function call between the lock and unlock
<wpwrak>
what i mean is that both are in a context where the can sleep. so why a spinlock ?
<drizzt>
only used in two places, with only tests and affectations between
<drizzt>
ha
<drizzt>
we can sleep outside the lock
<drizzt>
but not while holding the lock
<wpwrak>
sure, but since you can sleep, you may as well use a mutex to protect the data, no ?
<drizzt>
yes
<drizzt>
i already moved from spin_lock_irqsave to spin_lock
<wpwrak>
you have very complex locking in that driver. almost half of the code is some spin_lock/spin_unlock. this seems unusually hard.
<drizzt>
maybe I' mistaken, but they are as good as mutexes if the code inside does not sleep
<wpwrak>
well, you could aim for a lower density of locking operations
<drizzt>
most of the locking is around the list handling
<wpwrak>
yes. couldn't a single mutex handle this as well ? i.e., do you really need this sort of granularity
<wpwrak>
?
<drizzt>
and i tried a version with less locking, but got problems with the lists (oops on "poison" elements)
<drizzt>
and for the granularity, yes, the buffer for receiving data is very small, if i stay locked too long i loose data
<drizzt>
but a version with mutex may solve both problems
<drizzt>
you're right
<drizzt>
at least some lists do not need irqsave locking
<wpwrak>
the only obviously tricky bit seems to be davinci_gpio7_interrupt
<drizzt>
and I finally removed the list from the interrupt, replacing it by a flag
<drizzt>
yep
<drizzt>
I had list handling in there at the beginning
<wpwrak>
yup, you already decoupled this
<drizzt>
but no more
<wpwrak>
(had list handling there) hence the large number of spinlocks :)
<drizzt>
then the mutex should be better
<drizzt>
yep
<drizzt>
had to be irqsafe whenever handling lists
<wpwrak>
i'm not enirely sure about emb_intf_tty_receive_buf ... lemme see if i can find something
<drizzt>
no more now
<drizzt>
this one i must leave as is
<drizzt>
it's a tty lock
<drizzt>
at least it's what's done in tty drivers in the kernel (this parts has it's roots there)
<wpwrak>
okay, and it's decoupled, too. good.
<wpwrak>
now, if we could get rid of these global variables, it would be perfect
<drizzt>
these ?
<drizzt>
emb_intf ?
<wpwrak>
i'm not entirely sure, but my guess would be that you could put a pointer to emb_intf into driver_data
<drizzt>
there's only one, and it lives accross all the driver life
<wpwrak>
emb_intf, partial, i think there's more. you hid them well :)
<drizzt>
hum, yes
<wpwrak>
how about having multiple instances of the driver ? :)
<drizzt>
no
<drizzt>
only one instance of the tty line !
<drizzt>
that's a hard limit
<wpwrak>
it's still nice to make the code "proper"
<drizzt>
you've got one point :)
<drizzt>
there's also "sending"
<wpwrak>
particularly when submitting into mainline, there's always the risk of someone copying the example without being aware of the limitation
<drizzt>
ei_trace and ei_dbg, but these are for the "debug" version, and will go away with the debug code when the 8051 firmware is OK
<wpwrak>
yeah, i didn't look at the dbg things, like you said
<drizzt>
nice :)
<wpwrak>
embeded_intf_remove can lose one level of indentation as well. handle the if first and return immediately. voila :)
<drizzt>
ho, yes :(
<drizzt>
though when development will be over this won't be a module anymore
<wpwrak>
why not ?
<drizzt>
it's easier for development :)
<drizzt>
because it must be here before the system finds it's init and rootfs
<drizzt>
the 8051 is sending his first interrupts long before this, and with ne answer it goes into error
<wpwrak>
oh, i see. a case for initramfs ?
<drizzt>
we cope with this for the development, but the medical persons won't like the error led and bip to turn on at every startup
<drizzt>
no real incidence here (initramfs)
<drizzt>
the system boots on SD, and the module could be loader immediatly
<drizzt>
loaded
<wpwrak>
they're used to these devices that go BEEEEEEEP all the time :)
<drizzt>
but this is still too long, unless I manage to shorten the boot time
<wpwrak>
seems a little odd to have such a tight timeout
<wpwrak>
i mean, what good is the driver if the application isn't ready ?
<drizzt>
three seconds
<drizzt>
just to reply to the "heartbeat"
<wpwrak>
seems that the application should initiate the communication. the 8051 could time out a good while later.
<drizzt>
(it's one of the purposes of the interrupt)
<wpwrak>
well, have a longer timeout before the first heartbeat
<drizzt>
this is one of my requests to the 8051 firmware developper
<wpwrak>
ah, one more ting: the params .. they're binary in the data that gets sent around, right ?
<drizzt>
in the protocol ?
<drizzt>
between 8051 and kernel ?
<wpwrak>
yes
<drizzt>
they are ascci representations of the values
<drizzt>
to send 100, you must "write" the string "100"
<drizzt>
not 0x64
<drizzt>
hence the sprintf and scanf
<wpwrak>
oh, and fixed-format. not 100 but 0100,... ?
<drizzt>
yep
<drizzt>
"<d,01,0100>A01E" for example
<drizzt>
with even the CRC being sent "A01E" for 0xA01E !
<drizzt>
and i spent a lot of time before understanding that it's "A01E" and not "a01e"
<wpwrak>
oh dear. prt[8] ;-))
<drizzt>
?
<wpwrak>
i mean ptr
<drizzt>
prt[8] ?
<drizzt>
? ptr = ?
<drizzt>
lol ?
<wpwrak>
was this once ptr[4] ? ;-) then, when it crashed, you went to ptr[8] ? :) (in emb_intf_handle_cmd_set)
<drizzt>
ho, no
<drizzt>
I begun with 8
<drizzt>
and left it so
<wpwrak>
anyway, looks correct. just very odd, the whole thing. now i also understand why you have that memcpy there
<wpwrak>
i didn't spot anything evil in the rest. i'm a bit rusty on recent kernel interfaces and may have missed some atrocities, so you may want to try someone like christoph or lars for the 2nd run, after the basic cleanup
<drizzt>
ok
<drizzt>
I'll check the mutex for the lists
<drizzt>
but thanks a lot :)
<wpwrak>
you've logged the conversation ?
<drizzt>
please, send me a bill, or at least a way to send you the money
<drizzt>
yes
<drizzt>
it's all saved :)
<drizzt>
I'm paid for this job, so you should be for the review :)
<wpwrak>
(bill) i'll do the "paperwork" tomorrow. need to search for the stuff :)
<drizzt>
perfect
<wpwrak>
great. a bit of income is always a good change from the current 100% hobby work ;-)
<drizzt>
I may have one more important review to do, for small modifications, bet for mainline submitions in the future
<wpwrak>
i have an account in switzerland, so i'll deprive you of the opportunity of learning the strange rituals involved in sending money to argentina
<wpwrak>
review of this driver ? or something else ?
<drizzt>
it's for patches I made for Alcatel lucent, for usb-serial drivers
<drizzt>
and modifications to ppp, to reach theorical up and download speed on 3G, HSUPA and HSDPA networks
<drizzt>
they are under testing in their labs
<wpwrak>
wow. i can review it for style and general issues. for conflicts with the existing design, you'd have to get feedback directly from the people in charge
<drizzt>
moved from 25% to near 100% of theorical rates :)
<wpwrak>
not bad :)
<drizzt>
better than the other test suites thay had with other systems
<drizzt>
and in this case, they'll even pay me for the job of submitting to the kernel
<wpwrak>
that's how it should be
<drizzt>
:)
<drizzt>
yes
<drizzt>
so, I need to get a few hours of sleep (it's 4 am here)
<drizzt>
got to get up at 8
<wpwrak>
ah yes. and i should get some dinner. midnight :)
<drizzt>
so, "see" you tomorow :)
<drizzt>
++
<wpwrak>
sweet dreams ! :)
<wpwrak>
i hope all the off-topic stuff didn't scare the rest of the audience too much :) since a lot of it was actually general kernel style issues, i think it may have some use here, too
<drizzt>
hum, yes, we could have done it in private