Dear Holger,
Thanks for the commit on my previous patch.
Here is a new one:
This pacth makes hopping type (baseband or RF) a user changeable parameter.
Tested with:
- MetroSite: baseband and RF hopping works - UltraSite: RF hopping works
As previously, the default behavior of OpenBSC is not changed. If the configuration parameter is not provided, it will default to baseband hopping, as before.
It builds against the latest version of OpenBSC master.
Regards, Csaba
On Sat, Feb 07, 2015 at 06:33:30PM +0100, Sipos Csaba wrote:
Dear Holger,
Hi!
It builds against the latest version of OpenBSC master.
what is preferred is if you could already make a git commit and then use git send-email/git format-patch. Then I don't have to come up with a commit message. :)
wait_reset:1;
wait_reset:1,hopping_type:1;
What is the reasoning for the bitfield here? Do we want to save memory? Is it a packed structure on the network? I think I raised a point like this with the previous commit.
vty_out(vty, " nokia_site hopping-type %d%s", bts->nokia.hopping_type, VTY_NEWLINE);
In more recent times I prefer to go with string names instead of 0/1. E.g "hopping", "synthesizer".
+DEFUN(cfg_bts_nokia_site_hopping_type,
cfg_bts_nokia_site_hopping_type_cmd,"nokia_site hopping-type (0|1)",NOKIA_STR"Set the hopping type. 0=Baseband hopping, 1=Synthesizer (RF) hopping\n")
This is likely to cause a failure with the python end to end test. For the above command you need to write documentation for "nokia_site" "hopping-type" "0" "1". This means the help string needs to have four topics that end with a \n. I think the documentation for "0" and "1" is missing.
memcpy(fu_config + len, bts_config_2, sizeof(bts_config_2)); /* set hopping mode (Baseband and RF hopping work for the MetroSite) */
- if (need_hopping)
fu_config[len + 2 + 1] = 1; /* 0: no hopping, 1: Baseband hopping, 2: RF hopping */
- if (need_hopping) {
if(hopping_type != 1){LOGP(DNM, LOGL_NOTICE, "Baseband hopping selected!\n");fu_config[len + 2 + 1] = 1; /* 0: no hopping, 1: Baseband hopping, 2: RF hopping */}else{LOGP(DNM, LOGL_NOTICE, "Synthesizer (RF) hopping selected!\n");fu_config[len + 2 + 1] = 2; /* 0: no hopping, 1: Baseband hopping, 2: RF hopping */}- }
Maybe use an enum to represent the two hopping options. This way you can put the semantic/description into the enum and don't need to write it twice. If you go with names for configuring you can even directly map the values of the enum to "1" and "2" which will allow you to remove the branch.
For all of this please have a look at the value_string struct and how we go from string to number and number to string.
Hi,
What is the reasoning for the bitfield here?
This is a two state flag (0 or 1), there is no point store more than that. Besides, all the previous variables are used like this. If this is not OK, then all the variables work like this already in the code are also not OK (look at wait_reset, skip_reset, did_reset etc.) It sure saves some memory, but its more like the general rule of thumb not to use more memory than it is necessary for a specific variable.
In more recent times I prefer to go with string names instead of 0/1. E.g "hopping", "synthesizer".
Like my previous answer. I usually use the elements and coding style already considered OK (because it is in the code). If this is not OK, then a whole lot of the code is also not OK, and I don't feel my self up to refactoring all of it. What we can do is to go with this for now, and refactor the whole section later.
For the above command you need to write documentation
Will look into this. How can I test if it causes any problems? I tested the code and it builds without a single warning, but I never used this end to end python test.
Maybe use an enum to represent the two hopping options.
The same as the first and second answer. A lot of the alreday provided code is based on the previous bitfield approach for variable handling. I'd rather not change this on a per variable basis. I consider this a refactor task, and I don't think my coding skills are up to somethig like this. So if you don't mind I would stick to this for now, and will try and read a bit about what you suggested, and get back to this problem later, when I fully understand what needs to be done, and refactor all variables in the Nokia code to what you have in mind.
Regards, Csaba
----- Eredeti üzenet ----- Feladó: "Holger Hans Peter Freyther" holger@freyther.de Címzett: "Sipos Csaba" sipos.csaba@kvk.uni-obuda.hu Másolatot kap: openbsc@lists.osmocom.org Elküldött üzenetek: Vasárnap, 2015. Február. 8. 9:43:39 Tárgy: Re: [Patch]: Nokia: make hopping a user changeable parameter
On Sat, Feb 07, 2015 at 06:33:30PM +0100, Sipos Csaba wrote:
Dear Holger,
Hi!
It builds against the latest version of OpenBSC master.
what is preferred is if you could already make a git commit and then use git send-email/git format-patch. Then I don't have to come up with a commit message. :)
wait_reset:1;
wait_reset:1,hopping_type:1;
What is the reasoning for the bitfield here? Do we want to save memory? Is it a packed structure on the network? I think I raised a point like this with the previous commit.
vty_out(vty, " nokia_site hopping-type %d%s", bts->nokia.hopping_type, VTY_NEWLINE);
In more recent times I prefer to go with string names instead of 0/1. E.g "hopping", "synthesizer".
+DEFUN(cfg_bts_nokia_site_hopping_type,
cfg_bts_nokia_site_hopping_type_cmd,"nokia_site hopping-type (0|1)",NOKIA_STR"Set the hopping type. 0=Baseband hopping, 1=Synthesizer (RF) hopping\n")
This is likely to cause a failure with the python end to end test. For the above command you need to write documentation for "nokia_site" "hopping-type" "0" "1". This means the help string needs to have four topics that end with a \n. I think the documentation for "0" and "1" is missing.
memcpy(fu_config + len, bts_config_2, sizeof(bts_config_2)); /* set hopping mode (Baseband and RF hopping work for the MetroSite) */
- if (need_hopping)
fu_config[len + 2 + 1] = 1; /* 0: no hopping, 1: Baseband hopping, 2: RF hopping */
- if (need_hopping) {
if(hopping_type != 1){LOGP(DNM, LOGL_NOTICE, "Baseband hopping selected!\n");fu_config[len + 2 + 1] = 1; /* 0: no hopping, 1: Baseband hopping, 2: RF hopping */}else{LOGP(DNM, LOGL_NOTICE, "Synthesizer (RF) hopping selected!\n");fu_config[len + 2 + 1] = 2; /* 0: no hopping, 1: Baseband hopping, 2: RF hopping */}- }
Maybe use an enum to represent the two hopping options. This way you can put the semantic/description into the enum and don't need to write it twice. If you go with names for configuring you can even directly map the values of the enum to "1" and "2" which will allow you to remove the branch.
For all of this please have a look at the value_string struct and how we go from string to number and number to string.
On Sun, Feb 08, 2015 at 11:27:40AM +0100, Sipos Csaba wrote:
Hi,
Good Afternoon,
Will look into this. How can I test if it causes any problems? I tested the code and it builds without a single warning, but I never used this end to end python test.
well, you can manually test it by using the online help (press '?' after a part of the command). If it is empty then you have a problem.
The same as the first and second answer. A lot of the alreday provided code is based on the previous bitfield approach for variable handling. I'd rather not change this on a per variable basis. I consider this a refactor task, and I don't think my coding skills are up to somethig like this. So if you don't mind I would stick to this for now, and will try and read a bit about what you suggested, and get back to this problem later, when I fully understand what needs to be done, and refactor all variables in the Nokia code to what you have in mind.
if(hopping_type != 1){LOGP(DNM, LOGL_NOTICE, "Baseband hopping selected!\n");fu_config[len + 2 + 1] = 1; /* 0: no hopping, 1: Baseband hopping, 2: RF hopping */}else{LOGP(DNM, LOGL_NOTICE, "Synthesizer (RF) hopping selected!\n");fu_config[len + 2 + 1] = 2; /* 0: no hopping, 1: Baseband hopping, 2: RF hopping */}
I do mind and I am happy to help you to improve the patch. If you use an enum the above code will turn into
... function (enum nokia_hopping_type hopping_type, ..)
.. fu_config[len + 2 + 1] = hopping_type;
and when you search for the definition of the enum one will read
enum nokia_hopping_type { Nokia_HoppingNone = 0, Nokia_HoppingBaseband = 1, Nokia_HoppingRF = 2, };
When it comes to code the first priority should be to make it work and be expressive as possible. E.g. if you use enum's the semantic is part of the name (I don't need to know that in your case the 0 is mapped to 1 and a 1 mapped to the 2).
-
Holger Hans Peter Freyther -
Sipos Csaba