Discussion:
genesys backend status update
(too old to reply)
stef
2005-05-06 08:39:23 UTC
Permalink
Hello,

here's the status of the genesy backend after last commit:

- MD6471 is fully supported in color, gray and linart modes, at
50, 100, 150, 200, 250, 300, 400, 500, 600, 800 and 1200 dpi

- code have been split up so that the genesys_low.c file is now
unused, and replaced by genesys_gl646.c and genesys_gl841.c files.
These hold functions specific to their related ASIC.

- common code to both AISCs have been moved to genesys.c. "higher"
level functions calls code from genesys_gl646.c or genesys_gl841.c
based on the asic_type field which is initialized in
genesys_devices.c


Bugs left:

- scanner init fails if the previous scan got certain amount of
data. ie it doesn't fail allways. I'm trying to track it down,
but that's not easy. Unplugging/replugging the scanner solves it.
Once xsane has done a scan, the whole following session is OK.

Todo:

- for the MD6471, at dpi higher than 600, pixels are staggered.
It is how the scanner sends them. So after doing 'line distance'
calibration, this effect has then to be corrected (by shifting
columns).

Features to add:

- true hardware lineart will be added when we find a scanner doing it.
- add 16 bit scanning, which involves cancelling gamma calibration
in this case. Some tests I did show that images aren't of good
quality without hardware gamma correction.


I'd like the current backend been reviewed so that we know what would
be done to make it leave the experimental staging, and include it in the
regular CVS. I think it is almost ready, and is only lacking a man page.

Currently the gl841 file is a copy of the gl646, and needs quite some
work before having GL841 based scanners work. In the process, we may have
to add or move functions between genesys.c and low level files. My plan is
that genesys.c will keep all common functions, functions that differs only by
a few lines and so test the asic_field. Registers will have to be read or set
by using the genesys_read_reg_from_set() and genesys_set_reg_from_set().
The speficic files will contain all functions that heavily use
registers, or register of different meaning. Registers can be accessed directly
by using the anonymous enum defined at the top of these files (841 one needs
updating...).
Some of the existing functions that are in the low level files could be
split into a part that deals with registers, and another part of higher level
which could be moved to genesys.c . However, I think it's a bit early to do
it and that we'll handle such cases when enough gl841 code will have been
written.

Regards,
Stef
Henning Meier-Geinitz
2005-05-10 19:04:40 UTC
Permalink
Hi,
Post by stef
Hello,
By the way, thanks for all your work. So finally this backend came to
live despite (or because?) so many developers touched it but couldn't
finish it.
Post by stef
- MD6471 is fully supported in color, gray and linart modes, at
50, 100, 150, 200, 250, 300, 400, 500, 600, 800 and 1200 dpi
So you could mark it as "good" in genesys.desc. At the moment it is
still listed as "unsupported".

I just tried the UMAX Astra 4500 and it is still detected and moves
back its scan slider when I try to scan. But nothing more. It hangs
after that endlessly (well, at least more than 5 minutes). I'll attach
a debug log. Maybe, maybe, sometime in the future I'll have time again
to get it working.
Post by stef
I'd like the current backend been reviewed so that we know what would
be done to make it leave the experimental staging, and include it in the
regular CVS. I think it is almost ready, and is only lacking a man page.
Ok, so I tried to compile genesys using the instructions in README and
got the following warnings:

genesys.c: In function init_options':
genesys.c:3919: warning: unused variable has_ta'
genesys_gl646.c: In function gl646_bulk_full_size':
genesys_gl646.c:909: warning: unused parameter dev'
genesys_gl646.c: In function gl646_end_scan':
genesys_gl646.c:1499: warning: implicit declaration of function usleep'
genesys_gl841.c: In function gl841_bulk_full_size':
genesys_gl841.c:909: warning: unused parameter dev'
genesys_gl841.c: In function gl841_end_scan':
genesys_gl841.c:1499: warning: implicit declaration of function usleep'

Then I did "configure --enable-static; make; make libcheck" and got
the following:
Libraries exporting 'illegal' symbols:
*** backend/.libs/libsane-genesys.a:
00002c30 T genesys_adjust_gain
00002e50 T genesys_average_black
00002d00 T genesys_average_white
00001010 T genesys_bulk_read_data
00000e20 T genesys_bulk_write_data
00000bd0 T genesys_bulk_write_register
00002b70 T genesys_calculate_zmode
00002ae0 T genesys_calculate_zmode2
00001870 T genesys_create_slope_table
00001d00 T genesys_exposure_time
00001210 T genesys_fe_write_data
00001460 T genesys_get_address
000012d0 T genesys_get_status
00001fe0 T genesys_init_shading_data
000021b0 T genesys_read_data_from_scanner
00002a00 T genesys_read_feed_steps
00000910 T genesys_read_reg_from_set
00000ab0 T genesys_read_register
000022d0 T genesys_search_reference_point
00000d50 T genesys_set_buffer_address
00000940 T genesys_set_reg_from_set
00001490 T genesys_start_motor
000014c0 T genesys_stop_motor
00002100 T genesys_test_buffer_empty
00004be0 T genesys_wait_not_moving
00000740 T genesys_write_pnm_file
00000980 T genesys_write_register
00030210 D Gpo
00030240 D Motor
00030060 D Sensor
00000000 D Wolfson
00001b60 T gl646_begin_scan
000010e0 T gl646_bulk_full_size
000051e0 T gl646_coarse_gain_calibration
00001c30 T gl646_end_scan
00000100 T gl646_get_bitset_bit
00000040 T gl646_get_fast_feed_bit
00000080 T gl646_get_filter_bit
00000140 T gl646_get_gain4_bit
000000c0 T gl646_get_lineart_bit
00005d00 T gl646_init
000010f0 T gl646_init_registers
00002d60 T gl646_init_registers_for_coarse_calibration
00003870 T gl646_init_registers_for_scan
00003180 T gl646_init_registers_for_shading
00005510 T gl646_init_registers_for_warmup
000045a0 T gl646_offset_calibration
000021f0 T gl646_park_head
00002560 T gl646_search_start_position
00004350 T gl646_send_gamma_table
00001440 T gl646_send_slope_table
00001540 T gl646_set_fe
000018f0 T gl646_set_lamp_power
00001890 T gl646_set_motor_power
00001950 T gl646_set_powersaving
00001d50 T gl646_slow_back_home
00000180 T gl646_test_buffer_empty_bit
00000190 T gl646_test_motor_flag_bit
00001b60 T gl841_begin_scan
000010e0 T gl841_bulk_full_size
000051e0 T gl841_coarse_gain_calibration
00001c30 T gl841_end_scan
00000100 T gl841_get_bitset_bit
00000040 T gl841_get_fast_feed_bit
00000080 T gl841_get_filter_bit
00000140 T gl841_get_gain4_bit
000000c0 T gl841_get_lineart_bit
00005d00 T gl841_init
000010f0 T gl841_init_registers
00002d60 T gl841_init_registers_for_coarse_calibration
00003870 T gl841_init_registers_for_scan
00003180 T gl841_init_registers_for_shading
00005510 T gl841_init_registers_for_warmup
000045a0 T gl841_offset_calibration
000021f0 T gl841_park_head
00002560 T gl841_search_start_position
00004350 T gl841_send_gamma_table
00001440 T gl841_send_slope_table
00001540 T gl841_set_fe
000018f0 T gl841_set_lamp_power
00001890 T gl841_set_motor_power
00001950 T gl841_set_powersaving
00001d50 T gl841_slow_back_home
00000180 T gl841_test_buffer_empty_bit
00000190 T gl841_test_motor_flag_bit

Only sane_* or sanei_* should be supported. So if you really must export
functions (e.g. in genesys_gl841.c), name them sanei_genesys_* to
avoid clashes with other backends or frontends.

Ok, let's take a quick look at the code:
- better disable/#if 0 any code that uses printf or fprintf for debugging to
avoid trouble when running over the network or using scanimage
- all files that generate their own objects need to have
#include "../include/sane/config.h" as their first include.
- genesys.conf should also mention gl841 (even if not supported yet)

Once you are ready, don't forget to:

* update AUTHORS
* add the backend to sane.man
* move genesys.desc to doc/descriptions and to update it

As I've only had quick look you may want to recheck the points
mentioned in doc/backend-writing.txt before committing to CVS.

Bye,
Henning
stef
2005-05-11 06:12:01 UTC
Permalink
Hello,

I've started to address the points you raised (and I sohouldn't have
forgotten ...). I'll signal when I'll feel the backend is up to the requirements.

Regards,
Stef
Gerhard Jaeger
2005-05-11 07:55:22 UTC
Permalink
Post by stef
Hello,
I've started to address the points you raised (and I sohouldn't have
forgotten ...). I'll signal when I'll feel the backend is up to the requirements.
Regards,
Stef
Another thing, I've forgotten:
See the file genesys.c, i.e. function genesys_init()
static SANE_Status
genesys_init (Genesys_Device * dev)
{
switch (dev->model->asic_type)
{
case GENESYS_GL646:
return sanei_gl646_init (dev);
case GENESYS_GL841:
return sanei_gl841_init (dev);
}
return SANE_STATUS_INVAL;
}

which is called later on:
RIE (genesys_init (dev));

In a former post, I suggested to use a per ASIC/Model function pointer thing to
use, which could make life much more easier, that way, that you simply write:

dev->model->cmd_set->init(dev);

This will be done as follows:

We define a command set structure with all the functions needed (here
all functions that differ between GL646 and GL841):

struct genesys_cmd_set
{
SANE_Status (*init) (Genesys_Device * dev);
.
.
.
.
}

This command set structure then will be part of the Genesys_Model structure,
which is also part of the device structure...
This method is used intensively in gt68xx backend and I think we should
keep this also for the genesys backend as it allows easy extension of
the command set (think of adding also GL842)...

Gerhard
Gerhard Jaeger
2005-05-11 13:46:01 UTC
Permalink
Hi,

I've now finished the suggested restructure work for the genesys
backend - it can be found in experimental/genesys-new.
It's done the same (or nearly the same) way as Henning did it with
the gt68xx backend. The code is based on the current CVS stuff...

Any comments?
Gerhard
Philipp Schmid
2005-05-11 20:28:17 UTC
Permalink
Hello,
Post by Gerhard Jaeger
I've now finished the suggested restructure work for the genesys
backend - it can be found in experimental/genesys-new.
You've to keep in mind that genesys-new/genesys_gl841.c bases on my really bad version
of genesys_gl841.c that isn't able to get compiled. I'm sorry about this and this will
never happen again. Now I've fixed it, but only in genesys/ and not in genesys-new/.

Bye,
Philipp
Henning Meier-Geinitz
2005-05-28 19:00:45 UTC
Permalink
Hi,
Post by stef
I've started to address the points you raised (and I sohouldn't have
forgotten ...). I'll signal when I'll feel the backend is up to the requirements.
One more thing I just noticed: If you want to use the genesys.desc
file currently in experimental CVS, all the unsupported scanners in
the genesys.desc file that is present in the normal CVS must be moved
to unsupported.desc. That's what I prefer anyway.

Bye,
Henning

Gerhard Jaeger
2005-05-11 07:30:25 UTC
Permalink
Hi,
Post by Henning Meier-Geinitz
Hi,
Post by stef
Hello,
[SNIPSNAP]
Post by Henning Meier-Geinitz
Then I did "configure --enable-static; make; make libcheck" and got
00002c30 T genesys_adjust_gain
00002e50 T genesys_average_black
00002d00 T genesys_average_white
00001010 T genesys_bulk_read_data
00000e20 T genesys_bulk_write_data
00000bd0 T genesys_bulk_write_register
00002b70 T genesys_calculate_zmode
00002ae0 T genesys_calculate_zmode2
00001870 T genesys_create_slope_table
00001d00 T genesys_exposure_time
00001210 T genesys_fe_write_data
00001460 T genesys_get_address
000012d0 T genesys_get_status
00001fe0 T genesys_init_shading_data
000021b0 T genesys_read_data_from_scanner
00002a00 T genesys_read_feed_steps
00000910 T genesys_read_reg_from_set
00000ab0 T genesys_read_register
000022d0 T genesys_search_reference_point
00000d50 T genesys_set_buffer_address
00000940 T genesys_set_reg_from_set
00001490 T genesys_start_motor
000014c0 T genesys_stop_motor
00002100 T genesys_test_buffer_empty
00004be0 T genesys_wait_not_moving
00000740 T genesys_write_pnm_file
00000980 T genesys_write_register
00030210 D Gpo
00030240 D Motor
00030060 D Sensor
00000000 D Wolfson
00001b60 T gl646_begin_scan
000010e0 T gl646_bulk_full_size
000051e0 T gl646_coarse_gain_calibration
00001c30 T gl646_end_scan
00000100 T gl646_get_bitset_bit
00000040 T gl646_get_fast_feed_bit
00000080 T gl646_get_filter_bit
00000140 T gl646_get_gain4_bit
000000c0 T gl646_get_lineart_bit
00005d00 T gl646_init
000010f0 T gl646_init_registers
00002d60 T gl646_init_registers_for_coarse_calibration
00003870 T gl646_init_registers_for_scan
00003180 T gl646_init_registers_for_shading
00005510 T gl646_init_registers_for_warmup
000045a0 T gl646_offset_calibration
000021f0 T gl646_park_head
00002560 T gl646_search_start_position
00004350 T gl646_send_gamma_table
00001440 T gl646_send_slope_table
00001540 T gl646_set_fe
000018f0 T gl646_set_lamp_power
00001890 T gl646_set_motor_power
00001950 T gl646_set_powersaving
00001d50 T gl646_slow_back_home
00000180 T gl646_test_buffer_empty_bit
00000190 T gl646_test_motor_flag_bit
00001b60 T gl841_begin_scan
000010e0 T gl841_bulk_full_size
000051e0 T gl841_coarse_gain_calibration
00001c30 T gl841_end_scan
00000100 T gl841_get_bitset_bit
00000040 T gl841_get_fast_feed_bit
00000080 T gl841_get_filter_bit
00000140 T gl841_get_gain4_bit
000000c0 T gl841_get_lineart_bit
00005d00 T gl841_init
000010f0 T gl841_init_registers
00002d60 T gl841_init_registers_for_coarse_calibration
00003870 T gl841_init_registers_for_scan
00003180 T gl841_init_registers_for_shading
00005510 T gl841_init_registers_for_warmup
000045a0 T gl841_offset_calibration
000021f0 T gl841_park_head
00002560 T gl841_search_start_position
00004350 T gl841_send_gamma_table
00001440 T gl841_send_slope_table
00001540 T gl841_set_fe
000018f0 T gl841_set_lamp_power
00001890 T gl841_set_motor_power
00001950 T gl841_set_powersaving
00001d50 T gl841_slow_back_home
00000180 T gl841_test_buffer_empty_bit
00000190 T gl841_test_motor_flag_bit
Only sane_* or sanei_* should be supported. So if you really must export
functions (e.g. in genesys_gl841.c), name them sanei_genesys_* to
avoid clashes with other backends or frontends.
[SNIPSNAP]
that's why almost all backends, that consist of more than one file include
the other source files by the preprocessor to the main backend source-file.
That way you can keep everything "static".

In the genesys.c file you'll see that at least for genesys_devices.c.
lines 64ff:

#define BACKEND_NAME genesys

#include "../include/sane/sanei_backend.h"
#include "../include/sane/sanei_config.h"
#include "../include/sane/sanei_usb.h"

#include "genesys.h"
#include "genesys_devices.c"

This needs to be done for the other files too (what I tried):
#include "genesys.h"
#include "genesys_devices.c"
#include "genesys_low.c"
#include "genesys_gl646.c"
#include "genesys_gl841.c"

AND - wow, a lot of redefinitions etc - hell lot of work for cleanup, but
I think its necessary. Also these extern definitions could be skipped then...
Its IMHO also not necessary to have the register definition twice - possibly
the bit definitions on a per-regiser base, but that's all.

I just started with this "cleanup" and noticed i.e.
genesys_init, which is defined twice - which one should be called?

Is genesys_low.c still needed?

Stefane if you need a helping hand upon this work - let me know!

Gerhard
stef
2005-05-11 17:00:38 UTC
Permalink
Hello,

I don't think we can have gl641 and gl841 in the same file. First some
registers of same index have different bits. Second the enum we use to access
registers are different. This overlap make it much harder to have both in one file
than to have 2 cleanly separated objects. For instance since register sets are
packed differently (not the same amount of registers used), you wouldn't be
able to access them simply. You'll have to use the helper function, instead of
directly using indexes. Also you'll have defines of the same name which would have
to be of different values, and some values would need 2 different defines.

genesys_low.c is no more needed. There are currently a lot of redefinition,
but it is because the work on gl841 is at the beginning. Thing will really differ
in the future. Furthermor, I expect we will have problem with the Genesys_Gpo
struct because gl841 has more GPIOs, and I'm afraid that registers and bits to
set the frontend are different, and we then have troubles with Genesys_Sensor.

For the function pointer method, it seems clean. But we have to be sure
to the parameters will be allways the same (still thinking of Genesys_Gpo and
Genesys_Sensor). Which seems likely with the current state of the backend.
It would also decrease limit the number of exported functions. In fact we would
only need a gl646_init_funcs() and a gl841_init_funcs(). sanei_genesys_* would
still be needed but not sanei_glXXX_* .

I've look a little at genesys-new, and I don't feel the genesys_mid.c is
really needed. If I understood, it holds functions "between" low and high level,
do we really want these in an other file (which is included) ? I haven't the
feeling that genesys.C has grown too big. Maybe I am missing something ?

Currently, I'd be rather inclined to only add the function pointers to the
gensys backend and keep separated objects for low level.

Regards,
Stef
Gerhard Jaeger
2005-05-14 09:35:43 UTC
Permalink
Hi,

sorry for the late response.
Post by stef
Hello,
I don't think we can have gl641 and gl841 in the same file. First some
registers of same index have different bits. Second the enum we use to
access registers are different. This overlap make it much harder to have
both in one file than to have 2 cleanly separated objects. For instance
since register sets are packed differently (not the same amount of
registers used), you wouldn't be able to access them simply. You'll have to
use the helper function, instead of directly using indexes. Also you'll
have defines of the same name which would have to be of different values,
and some values would need 2 different defines.
Probably you're right here, but when having "locally" exported functions,
don't name it sanei_...,
Post by stef
genesys_low.c is no more needed. There are currently a lot of
redefinition, but it is because the work on gl841 is at the beginning.
Thing will really differ in the future. Furthermor, I expect we will have
problem with the Genesys_Gpo struct because gl841 has more GPIOs, and I'm
afraid that registers and bits to set the frontend are different, and we
then have troubles with Genesys_Sensor.
These issues, I think have been solved in genesys_new. Anyway as I told
you I don't insist on continuing with genesys_new, I simply did this
work to demonstrate how it could be done with the function pointers
(which - as far as I can see - you have now implemented...)
Post by stef
For the function pointer method, it seems clean. But we have to be sure
to the parameters will be allways the same (still thinking of Genesys_Gpo
and Genesys_Sensor). Which seems likely with the current state of the
backend.
Guess this is not really a problem.
Post by stef
It would also decrease limit the number of exported functions. In
fact we would only need a gl646_init_funcs() and a gl841_init_funcs().
sanei_genesys_* would still be needed but not sanei_glXXX_* .
I've look a little at genesys-new, and I don't feel the genesys_mid.c is
really needed. If I understood, it holds functions "between" low and high
level, do we really want these in an other file (which is included) ? I
haven't the feeling that genesys.C has grown too big. Maybe I am missing
something ?
Hmmm, I do really think, that having more than 4000 lines of code is pretty
too much for one file. One other thing are the gamma settings for the
various models, couldn't these values be generated by a function?
I did some tests and something like:

for( i = 0, c = 0; i<gamma_len; i++ ) {

val = ( gamma_max * pow((double)i / (double)( gamma_len-1.0), 1.0 / gamma ));

if( val > gamma_max )
val = gamma_max;

for( j=0; j<4; j++ )
gamma_table[c++] = val;
}

generates the table: gamma = 2.2, gamma_len = 4096, gamma_max = 16380;
Post by stef
Currently, I'd be rather inclined to only add the function pointers to the
gensys backend and keep separated objects for low level.
Okay...
Post by stef
Regards,
Stef
Anyway, keep up the good work, I currently check the ST12 and ST24 stuff.

Ciao,
Gerhard
stef
2005-05-20 15:07:51 UTC
Permalink
Post by Gerhard Jaeger
Hi,
sorry for the late response.
Hello, seems I'm not also that fast on mail. I was busy getting
2300c to work ...
Post by Gerhard Jaeger
Post by stef
Hello,
I don't think we can have gl641 and gl841 in the same file. First some
registers of same index have different bits. Second the enum we use to
access registers are different. This overlap make it much harder to have
both in one file than to have 2 cleanly separated objects. For instance
since register sets are packed differently (not the same amount of
registers used), you wouldn't be able to access them simply. You'll have to
use the helper function, instead of directly using indexes. Also you'll
have defines of the same name which would have to be of different values,
and some values would need 2 different defines.
Probably you're right here, but when having "locally" exported functions,
don't name it sanei_...,
Post by stef
genesys_low.c is no more needed. There are currently a lot of
redefinition, but it is because the work on gl841 is at the beginning.
Thing will really differ in the future. Furthermor, I expect we will have
problem with the Genesys_Gpo struct because gl841 has more GPIOs, and I'm
afraid that registers and bits to set the frontend are different, and we
then have troubles with Genesys_Sensor.
These issues, I think have been solved in genesys_new. Anyway as I told
you I don't insist on continuing with genesys_new, I simply did this
work to demonstrate how it could be done with the function pointers
(which - as far as I can see - you have now implemented...)
Post by stef
For the function pointer method, it seems clean. But we have to be sure
to the parameters will be allways the same (still thinking of Genesys_Gpo
and Genesys_Sensor). Which seems likely with the current state of the
backend.
Guess this is not really a problem.
Post by stef
It would also decrease limit the number of exported functions. In
fact we would only need a gl646_init_funcs() and a gl841_init_funcs().
sanei_genesys_* would still be needed but not sanei_glXXX_* .
I've look a little at genesys-new, and I don't feel the genesys_mid.c is
really needed. If I understood, it holds functions "between" low and high
level, do we really want these in an other file (which is included) ? I
haven't the feeling that genesys.C has grown too big. Maybe I am missing
something ?
Hmmm, I do really think, that having more than 4000 lines of code is pretty
too much for one file. One other thing are the gamma settings for the
I'm not personnaly keen on splitting files and then including them.
However, personal taste is not a sensible argument.
Including source files would involve adding an option to makedepend
in the makefile (we want 'makedepend -o.lo *.c') so that dependencies are
correctly handled. That raise one question, is there a build environnment where
sane objects are built with .o suffixes ? I couldn't find out it from the
makefiles or config files.
Post by Gerhard Jaeger
various models, couldn't these values be generated by a function?
for( i = 0, c = 0; i<gamma_len; i++ ) {
val = ( gamma_max * pow((double)i / (double)( gamma_len-1.0), 1.0 / gamma ));
if( val > gamma_max )
val = gamma_max;
for( j=0; j<4; j++ )
gamma_table[c++] = val;
}
generates the table: gamma = 2.2, gamma_len = 4096, gamma_max = 16380;
Post by stef
Currently, I'd be rather inclined to only add the function pointers to the
gensys backend and keep separated objects for low level.
Gamma tables are currently a nobrainer cut'n past from usb logs. I've
started to see if they can be replace by an init functions. However, the MD6471
case seems a little more complicated than a pow() formula.
Post by Gerhard Jaeger
Okay...
Post by stef
Regards,
Stef
Anyway, keep up the good work, I currently check the ST12 and ST24 stuff.
Ciao,
Gerhard
After latest check-ins, the HP 2300c is now well supported, and isn't
experimental anymore, like MD5345/MD6228/MD6471.

What I am looking into now, is to see if we can get rid of the 'static'
gamma tables, adding true lineart scanning and 16 bits scanning. I feel
like addng a 16bit option and a 'channel filter' select option.

Regards,
Stef
Loading...