Thanks Seb, this helps a lot. Matt.
On Jan 19, 5:15 am, [email protected] wrote: > Comment by sebastien.lelong: > > General Comment: > Hi Matt, > > Some comments about your libs. Maybe there are duplicates of what you > lately mentioned in jallib thread. > > Cheers, > Seb > > Line-by-line comments: > > File: /trunk/include/networking/ethernet_mac.jal (r2437) > =============================================================================== > > Line 26: procedure ethernet_header_create(byte in type, byte in mac_0, byte > in mac_1, byte in mac_2, byte in mac_3, byte in mac_4, byte in mac_5) is > ------------------------------------------------------------------------------- > About "type" argument. I understand it's used to specify the last byte of > the two-byte ethertype field. First byte is hard-coded to 0x08. "type" can > be 0x00 so ethertype = 0x0800 for IPv4 frame (eg ICMP). It can be 0x06 to > match ARP protocol where ethertype is 0x0806. > > Why didn't you allow "type" to be a word and let callee specify the full > ethertype value ? ethernet_header_create() is called with named > constant "ARP" within arp.jal lib, and with not-so-meaningful 0 value > within icmp.jal lib for instance. Does it make more sense to say > ethernet_header_create(IPv4,...) or ethernet_header_create(ARP,...) and > define const word IPv4 = 0x0800 and const word ARP = 0x0806. It think it > would be easier to follow/read the code, and would also allow other > ethertype to be implemented (who knows...) > > Line 27: -- set source and destination MAC addresses > ------------------------------------------------------------------------------- > Nitpicking... Follow ethernet frame fields order: first destination, then > source. this way reader can network_data[0], ...[1], ..., ...[5], and then > better understand why you have an offset of 6 to set source address. > > As I said, nitpicking... > File: /trunk/include/networking/network_globals.jal (r2437) > =============================================================================== > > Line 49: if NETWORK_ENC28J60 == TRUE then > ------------------------------------------------------------------------------- > Same remark as for NETWORK_LINK_LAYER, if you have multiple ethernet > modules, other than ENC28J60, True/False approach won't work anymore. > File: /trunk/include/networking/network_main.jal (r2437) > =============================================================================== > > Line 71: if NETWORK_LINK_LAYER_ETHERNET == FALSE then > ------------------------------------------------------------------------------- > From what I understand, when NETWORK_LINK_LAYER_ETHERNET is False, this > means you're using SLIP. If another protocol is implements, this True/False > approach won't work anymore (or with a lot of "if"). > > What about checking NETWORK_LINK_LAYER = ETHERNET / SLIP, that is, checking > against some arbitrary constant value ? > > For more information:http://code.google.com/p/jallib/source/detail?r=2437 -- You received this message because you are subscribed to the Google Groups "jallib" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/jallib?hl=en.
